Merge pull request #299 from Bplotka/fix/issue3943

compact: Assume fresh block to ignore by minTime not ULID order. Added tests.

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 {
@@ -176,6 +171,10 @@ func (c *LeveledCompactor) plan(dms []dirMeta) ([]string, error) {
 		return dms[i].meta.MinTime < dms[j].meta.MinTime
 	})
 
+	// We do not include a recently created block with max(minTime), so 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 = dms[:len(dms)-1]
+
 	var res []string
 	for _, dm := range c.selectDirs(dms) {
 		res = append(res, dm.dir)

compact_test.go

@@ -153,12 +153,13 @@ func TestNoPanicFor0Tombstones(t *testing.T) {
 }
 
 func TestLeveledCompactor_plan(t *testing.T) {
+	// This mimicks our default ExponentialBlockRanges with min block size equals to 20.
 	compactor, err := NewLeveledCompactor(nil, nil, []int64{
 		20,
 		60,
-		240,
+		180,
-		720,
+		540,
-		2160,
+		1620,
 	}, nil)
 	testutil.Ok(t, err)
 
@@ -172,8 +173,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 +181,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 from WAl.
+		{
+			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. "5" is marked as fresh one.
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
@@ -211,40 +234,45 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			expected: []string{"1", "2", "3"},
 		},
 		{
+			// We have 20, 60, 20, 60, 240 range blocks. We can compact 20 + 60 + 60.
 			metas: []dirMeta{
 				metaRange("2", 20, 40, nil),
 				metaRange("4", 60, 120, nil),
-				metaRange("5", 120, 180, nil),
+				metaRange("5", 960, 980, nil), // Fresh one.
-				metaRange("6", 720, 960, nil),
+				metaRange("6", 120, 180, nil),
+				metaRange("7", 720, 960, nil),
 			},
-			expected: []string{"2", "4", "5"},
+			expected: []string{"2", "4", "6"},
 		},
+		// Do not select large blocks that have many tombstones when there is no fresh block.
 		{
 			metas: []dirMeta{
-				metaRange("1", 0, 60, nil),
+				metaRange("1", 0, 540, &BlockStats{
-				metaRange("4", 60, 80, nil),
-				metaRange("5", 80, 100, nil),
-				metaRange("6", 100, 120, nil),
-			},
-			expected: []string{"4", "5", "6"},
-		},
-		// Select large blocks that have many tombstones.
-		{
-			metas: []dirMeta{
-				metaRange("1", 0, 720, &BlockStats{
 					NumSeries:     10,
 					NumTombstones: 3,
 				}),
 			},
+			expected: nil,
+		},
+		// Select large blocks that have many tombstones when fresh appears.
+		{
+			metas: []dirMeta{
+				metaRange("1", 0, 540, &BlockStats{
+					NumSeries:     10,
+					NumTombstones: 3,
+				}),
+				metaRange("2", 540, 560, 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{
+				metaRange("1", 0, 60, &BlockStats{
 					NumSeries:     10,
 					NumTombstones: 3,
 				}),
+				metaRange("2", 60, 80, nil),
 			},
 			expected: nil,
 		},
@@ -252,20 +280,39 @@ func TestLeveledCompactor_plan(t *testing.T) {
 		// the same block when tombstones and series counts were zero.
 		{
 			metas: []dirMeta{
-				metaRange("1", 0, 720, &BlockStats{
+				metaRange("1", 0, 540, &BlockStats{
 					NumSeries:     0,
 					NumTombstones: 0,
 				}),
+				metaRange("2", 540, 560, nil),
 			},
 			expected: nil,
 		},
+		// Regression test: we were wrongly assuming that new block is fresh from WAL when its ULID is newest.
+		// We need to actually look on max time instead.
+		//
+		// With previous, wrong approach "8" block was ignored, so we were wrongly compacting 5 and 7 and introducing
+		// block overlaps.
+		{
+			metas: []dirMeta{
+				metaRange("5", 0, 360, nil),
+				metaRange("6", 540, 560, nil), // Fresh one.
+				metaRange("7", 360, 420, nil),
+				metaRange("8", 420, 540, nil),
+			},
+			expected: []string{"7", "8"},
+		},
 	}
 
 	for _, c := range cases {
-		res, err := compactor.plan(c.metas)
+		if !t.Run("", func(t *testing.T) {
-		testutil.Ok(t, err)
+			res, err := compactor.plan(c.metas)
+			testutil.Ok(t, err)
 
-		testutil.Equals(t, c.expected, res)
+			testutil.Equals(t, c.expected, res)
+		}) {
+			return
+		}
 	}
 }
 
@@ -287,6 +334,7 @@ func TestRangeWithFailedCompactionWontGetSelected(t *testing.T) {
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
 				metaRange("3", 40, 60, nil),
+				metaRange("4", 60, 80, nil),
 			},
 		},
 		{
@@ -294,6 +342,7 @@ func TestRangeWithFailedCompactionWontGetSelected(t *testing.T) {
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
 				metaRange("3", 60, 80, nil),
+				metaRange("4", 80, 100, nil),
 			},
 		},
 		{
@@ -303,6 +352,7 @@ func TestRangeWithFailedCompactionWontGetSelected(t *testing.T) {
 				metaRange("3", 40, 60, nil),
 				metaRange("4", 60, 120, nil),
 				metaRange("5", 120, 180, nil),
+				metaRange("6", 180, 200, nil),
 			},
 		},
 	}