Only consider an interval to have possible expired chunks if it overl…

…aps a delete. (#6297)
grafana · Jun 3, 2022 · ea0a524 · ea0a524
1 parent cd7ebf2
commit ea0a524
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 2 deletions.
diff --git a/pkg/storage/stores/shipper/compactor/deletion/delete_requests_manager.go b/pkg/storage/stores/shipper/compactor/deletion/delete_requests_manager.go
@@ -203,13 +203,15 @@ func (d *DeleteRequestsManager) MarkPhaseFinished() {
 	}
 }
 
-func (d *DeleteRequestsManager) IntervalMayHaveExpiredChunks(_ model.Interval, userID string) bool {
+func (d *DeleteRequestsManager) IntervalMayHaveExpiredChunks(interval model.Interval, userID string) bool {
 	d.deleteRequestsToProcessMtx.Lock()
 	defer d.deleteRequestsToProcessMtx.Unlock()
 
 	if userID != "" {
 		for _, deleteRequest := range d.deleteRequestsToProcess {
-			if deleteRequest.UserID == userID {
+			if deleteRequest.UserID == userID &&
+				deleteRequest.StartTime <= interval.End &&
+				deleteRequest.EndTime >= interval.Start {
 				return true
 			}
 		}

diff --git a/pkg/storage/stores/shipper/compactor/deletion/delete_requests_manager_test.go b/pkg/storage/stores/shipper/compactor/deletion/delete_requests_manager_test.go
@@ -304,6 +304,31 @@ func TestDeleteRequestsManager_Expired(t *testing.T) {
 	}
 }
 
+func TestDeleteRequestsManager_IntervalMayHaveExpiredChunks(t *testing.T) {
+	tt := []struct {
+		deleteRequestsFromStore []DeleteRequest
+		hasChunks               bool
+		user                    string
+	}{
+		{[]DeleteRequest{{Query: `0`, UserID: "test-user", StartTime: 0, EndTime: 100}}, false, "test-user"},
+		{[]DeleteRequest{{Query: `1`, UserID: "test-user", StartTime: 200, EndTime: 400}}, true, "test-user"},
+		{[]DeleteRequest{{Query: `2`, UserID: "test-user", StartTime: 400, EndTime: 500}}, true, "test-user"},
+		{[]DeleteRequest{{Query: `3`, UserID: "test-user", StartTime: 500, EndTime: 700}}, true, "test-user"},
+		{[]DeleteRequest{{Query: `3`, UserID: "other-user", StartTime: 500, EndTime: 700}}, false, "test-user"},
+		{[]DeleteRequest{{Query: `4`, UserID: "test-user", StartTime: 700, EndTime: 900}}, false, "test-user"},
+		{[]DeleteRequest{{Query: `4`, UserID: "", StartTime: 700, EndTime: 900}}, true, ""},
+		{[]DeleteRequest{}, false, ""},
+	}
+
+	for _, tc := range tt {
+		mgr := NewDeleteRequestsManager(mockDeleteRequestsStore{deleteRequests: tc.deleteRequestsFromStore}, time.Hour, nil, FilterAndDelete)
+		require.NoError(t, mgr.loadDeleteRequestsToProcess())
+
+		interval := model.Interval{Start: 300, End: 600}
+		require.Equal(t, tc.hasChunks, mgr.IntervalMayHaveExpiredChunks(interval, tc.user))
+	}
+}
+
 type mockDeleteRequestsStore struct {
 	DeleteRequestsStore
 	deleteRequests []DeleteRequest