compact: Exclude last block with shortest range instead of newest one by ULID.

Fixes https://github.com/prometheus/prometheus/issues/3943 issue.

Added tests.

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

compact.go

@@ -151,16 +151,11 @@ func (c *LeveledCompactor) Plan(dir string) ([]string, error) {
 	if err != nil {
 		return nil, err
 	}
-	// We do not include the most recently created block. This gives users a window
-	// of a full block size to piece-wise backup new data without having to care
-	// about data overlap.
 	if len(dirs) < 1 {
 		return nil, nil
 	}
-	dirs = dirs[:len(dirs)-1]
 
 	var dms []dirMeta
-
 	for _, dir := range dirs {
 		meta, err := readMetaFile(dir)
 		if err != nil {
@@ -172,6 +167,10 @@ func (c *LeveledCompactor) Plan(dir string) ([]string, error) {
 }
 
 func (c *LeveledCompactor) plan(dms []dirMeta) ([]string, error) {
+	// We do not include the most recently created block with the shortest range (the block which was just created from WAL).
+	// This gives users a window of a full block size to piece-wise backup new data without having to care about data overlap.
+	dms = excludeFreshMeta(dms)
+
 	sort.Slice(dms, func(i, j int) bool {
 		return dms[i].meta.MinTime < dms[j].meta.MinTime
 	})
@@ -199,6 +198,20 @@ func (c *LeveledCompactor) plan(dms []dirMeta) ([]string, error) {
 	return nil, nil
 }
 
+// excludeFreshMeta removes newest block with the shortest range from given meta files.
+func excludeFreshMeta(dms []dirMeta) []dirMeta {
+	sort.Slice(dms, func(i, j int) bool {
+		leftRange := dms[i].meta.MaxTime - dms[i].meta.MinTime
+		rightRange := dms[j].meta.MaxTime - dms[j].meta.MinTime
+		if leftRange == rightRange {
+			return dms[i].meta.MinTime < dms[j].meta.MinTime
+		}
+
+		return leftRange > rightRange
+	})
+	return dms[:len(dms)-1]
+}
+
 // selectDirs returns the dir metas that should be compacted into a single new block.
 // If only a single block range is configured, the result is always nil.
 func (c *LeveledCompactor) selectDirs(ds []dirMeta) []dirMeta {

compact_test.go

@@ -172,8 +172,7 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: nil,
 		},
-		// We should wait for a third block of size 20 to appear before compacting
+		// We should wait for four blocks of size 20 to appear before compacting.
-		// the existing ones.
 		{
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
@@ -181,26 +180,49 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: nil,
 		},
+		// We should wait for a next block of size 20 to appear before compacting
+		// the existing ones. We have three, but we ignore the fresh one with shortest range.
+		{
+			metas: []dirMeta{
+				metaRange("1", 0, 20, nil),
+				metaRange("2", 20, 40, nil),
+				metaRange("3", 40, 60, nil),
+			},
+			expected: nil,
+		},
 		// Block to fill the entire parent range appeared – should be compacted.
 		{
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
 				metaRange("3", 40, 60, nil),
+				metaRange("4", 60, 80, nil),
 			},
 			expected: []string{"1", "2", "3"},
 		},
-		// Block for the next parent range appeared. Nothing will happen in the first one
+		// Block for the next parent range appeared with gap with size 20. Nothing will happen in the first one
-		// anymore and we should compact it.
+		// anymore but we ignore fresh one still, so no compaction.
 		{
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
 				metaRange("3", 60, 80, nil),
 			},
+			expected: nil,
+		},
+		// Block for the next parent range appeared, and we have a gap with size 20 between second and third block.
+		// We will not get this missed gap anymore and we should compact just these two.
+		{
+			metas: []dirMeta{
+				metaRange("1", 0, 20, nil),
+				metaRange("2", 20, 40, nil),
+				metaRange("3", 60, 80, nil),
+				metaRange("4", 80, 100, nil),
+			},
 			expected: []string{"1", "2"},
 		},
 		{
+			// We have 20, 20, 20, 60, 60 range blocks. "3" is marked as fresh one, so we ignore it during compaction.
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
@@ -208,27 +230,54 @@ func TestLeveledCompactor_plan(t *testing.T) {
 				metaRange("4", 60, 120, nil),
 				metaRange("5", 120, 180, nil),
 			},
-			expected: []string{"1", "2", "3"},
+			expected: []string{"1", "2"},
 		},
 		{
+			// We have 20, 60, 60, 240 range blocks. We can compact 60 + 60 only, because the newest one with the
+			// shortest range is not included in planning (4).
 			metas: []dirMeta{
 				metaRange("2", 20, 40, nil),
 				metaRange("4", 60, 120, nil),
 				metaRange("5", 120, 180, nil),
 				metaRange("6", 720, 960, nil),
 			},
+			expected: []string{"4", "5"},
+		},
+		{
+			// We got fresh block if size 20. We can compact 20 + 60 + 60.
+			metas: []dirMeta{
+				metaRange("2", 20, 40, nil),
+				metaRange("4", 60, 120, nil),
+				metaRange("5", 120, 180, nil),
+				metaRange("6", 720, 960, nil),
+				metaRange("7", 960, 980, nil),
+			},
 			expected: []string{"2", "4", "5"},
 		},
 		{
+			// We have 60, 20, 20, 20 range blocks.
+			// Fresh block with shortest range block is ignored (6), so nothing can be compacted.
 			metas: []dirMeta{
 				metaRange("1", 0, 60, nil),
 				metaRange("4", 60, 80, nil),
 				metaRange("5", 80, 100, nil),
 				metaRange("6", 100, 120, nil),
 			},
+			expected: nil,
+		},
+		{
+			// We have 60, 20, 20, 20, 20 range blocks.
+			// Fresh block with shortest range block is ignored but compaction 20 + 20 + 20 is possible.
+			metas: []dirMeta{
+				metaRange("1", 0, 60, nil),
+				metaRange("4", 60, 80, nil),
+				metaRange("5", 80, 100, nil),
+				metaRange("6", 100, 120, nil),
+				metaRange("6", 120, 140, nil),
+			},
 			expected: []string{"4", "5", "6"},
 		},
-		// Select large blocks that have many tombstones.
+		// Do not select large blocks that have many tombstones when there is no fresh block.
 		{
 			metas: []dirMeta{
 				metaRange("1", 0, 720, &BlockStats{
@@ -236,15 +285,27 @@ func TestLeveledCompactor_plan(t *testing.T) {
 					NumTombstones: 3,
 				}),
 			},
+			expected: nil,
+		},
+		// Select large blocks that have many tombstones when fresh appears.
+		{
+			metas: []dirMeta{
+				metaRange("1", 0, 720, &BlockStats{
+					NumSeries:     10,
+					NumTombstones: 3,
+				}),
+				metaRange("2", 720, 740, nil),
+			},
 			expected: []string{"1"},
 		},
-		// For small blocks, do not compact tombstones.
+		// For small blocks, do not compact tombstones, even when fresh appears.
 		{
 			metas: []dirMeta{
 				metaRange("1", 0, 30, &BlockStats{
 					NumSeries:     10,
 					NumTombstones: 3,
 				}),
+				metaRange("2", 720, 740, nil),
 			},
 			expected: nil,
 		},
@@ -256,16 +317,21 @@ func TestLeveledCompactor_plan(t *testing.T) {
 					NumSeries:     0,
 					NumTombstones: 0,
 				}),
+				metaRange("2", 720, 740, nil),
 			},
 			expected: nil,
 		},
 	}
 
 	for _, c := range cases {
+		if !t.Run("", func(t *testing.T) {
 			res, err := compactor.plan(c.metas)
 			testutil.Ok(t, err)
 
 			testutil.Equals(t, c.expected, res)
+		}) {
+			return
+		}
 	}
 }