Small optimization to avoid unnecessary Extents merge when growing cache entry (#4026)

* Small optimization to avoid unnecessary Extents merge when growing cache entry

Signed-off-by: Alvin Lin <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Alvin Lin <[email protected]>

* Remove unintended whitespace modification

Signed-off-by: Alvin Lin <[email protected]>

* Add test to indirectly test the existence of the optimization

Signed-off-by: Alvin Lin <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Marco Pracucci <[email protected]>

Co-authored-by: Marco Pracucci <[email protected]>

Author: alvinlin123

CHANGELOG.md

@@ -25,6 +25,7 @@
 * [ENHANCEMENT] Distributor: reduce CPU and memory when an high number of errors are returned by the distributor on the write path. #3990
 * [ENHANCEMENT] Put metric before label value in the "label value too long" error message. #4018
 * [ENHANCEMENT] Allow use of `y|w|d` suffixes for duration related limits and per-tenant limits. #4044
+* [ENHANCEMENT] Query-frontend: Small optimization on top of PR #3968 to avoid unnecessary Extents merging. #4026
 * [BUGFIX] Ruler-API: fix bug where `/api/v1/rules/<namespace>/<group_name>` endpoint return `400` instead of `404`. #4013
 * [BUGFIX] Distributor: reverted changes done to rate limiting in #3825. #3948
 * [BUGFIX] Ingester: Fix race condition when opening and closing tsdb concurrently. #3959

pkg/querier/queryrange/results_cache.go

@@ -385,6 +385,13 @@ func (s resultsCache) handleHit(ctx context.Context, r Request, extents []Extent
 		extents = append(extents, extent)
 	}
 	sort.Slice(extents, func(i, j int) bool {
+		if extents[i].Start == extents[j].Start {
+			// as an optimization, for two extents starts at the same time, we
+			// put bigger extent at the front of the slice, which helps
+			// to reduce the amount of merge we have to do later.
+			return extents[i].End > extents[j].End
+		}
+
 		return extents[i].Start < extents[j].Start
 	})
 

pkg/querier/queryrange/results_cache_test.go

@@ -690,6 +690,31 @@ func TestHandleHit(t *testing.T) {
 				mkExtentWithStep(60, 180, 20),
 			},
 		},
+		{
+			name: "Should not throw error if complete-overlapped smaller Extent is erroneous",
+			input: &PrometheusRequest{
+				// This request is carefully crated such that cachedEntry is not used to fulfill
+				// the request.
+				Start: 160,
+				End:   180,
+				Step:  20,
+			},
+			cachedEntry: []Extent{
+				{
+					Start: 60,
+					End:   80,
+
+					// if the optimization of "sorting by End when Start of 2 Extents are equal" is not there, this nil
+					// response would cause error during Extents merge phase. With the optimization
+					// this bad Extent should be dropped. The good Extent below can be used instead.
+					Response: nil,
+				},
+				mkExtentWithStep(60, 160, 20),
+			},
+			expectedUpdatedCachedEntry: []Extent{
+				mkExtentWithStep(60, 180, 20),
+			},
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			sut := resultsCache{