feat: Modify optimized compaction to cover edge cases #25594

devanbenz · 2024-11-27T21:21:47Z

This PR changes the algorithm for compaction to account for the following
cases that were not previously accounted for:

Many generations with a groupsize over 2 GB
Single generation with many files and a groupsize under 2 GB

Where groupsize is the total size of the TSM files in said shard directory.

tsdb/engine/tsm1/compact.go

tsdb/engine/tsm1/engine.go

tsdb/engine/tsm1/compact.go

gwossum

First pass, mostly about comments.

This PR changes the algorithm for compaction to account for the following cases that were not previously accounted for: - Many generations with a groupsize over 2 GB - Single generation with many files and a groupsize under 2 GB Where groupsize is the total size of the TSM files in said shard directory. closes #25666

davidby-influx

I still need to review the tests more closely, as well.

tsdb/engine/tsm1/compact.go

tsdb/engine/tsm1/compact_test.go

for shards that may have over a 2 GB group size but many fragmented files (under 2 GB and under aggressive point per block count)

davidby-influx

Lots of good changes and more tests! Thanks for the effort.
I still have a few things that you may want to change. Happy to discuss things in a teleconference.

tsdb/config.go

tsdb/engine/tsm1/compact.go

tsdb/engine/tsm1/compact_test.go

davidby-influx

Great changes, really improving this code. A few more in the endless cycle...

tsdb/engine/tsm1/compact.go

tsdb/engine/tsm1/compact_test.go

davidby-influx · 2024-12-18T18:21:58Z

tsdb/config.go

@@ -77,6 +81,9 @@ const (
 	// partition snapshot compactions that can run at one time.
 	// A value of 0 results in runtime.GOMAXPROCS(0).
 	DefaultSeriesFileMaxConcurrentSnapshotCompactions = 0
+
+	// MaxTSMFileSize is the maximum size of TSM files.


tsdb/engine/tsm1/compact_test.go

davidby-influx

Code looks good, suggested more test scenarios.

Think about backfills, and the files they could generate.

tsdb/engine/tsm1/compact_test.go

…and mixed block counts

davidby-influx

Looks good. Please don't merge until @gwossum has a chance to re-review, though

davidby-influx

Overall, I am confused by the conditionals in the planner that compare a metric from the first file in a generation and also compare aggregate metrics of the generation. Either there is some invariant (e.g., the first file is always the largest) which is neither tested nor documented, or these conditionals are heuristic, not reliable.

The comments seems to refer to single files, but the operations are on generations. Maybe I'm missing something. Let's discuss.

If there is an invariant (like the first file in a generation is always the largest), let's both document it and test for it.

davidby-influx · 2025-01-08T23:00:01Z

tsdb/engine/tsm1/compact.go

@@ -454,7 +474,7 @@ func (c *DefaultPlanner) Plan(lastWrite time.Time) ([]CompactionGroup, int64) {
 			var skip bool

 			// Skip the file if it's over the max size and contains a full block and it does not have any tombstones
-			if len(generations) > 2 && group.size() > uint64(maxTSMFileSize) && c.FileStore.BlockCount(group.files[0].Path, 1) == tsdb.DefaultMaxPointsPerBlock && !group.hasTombstones() {
+			if len(generations) > 2 && group.size() > uint64(tsdb.MaxTSMFileSize) && c.FileStore.BlockCount(group.files[0].Path, 1) >= tsdb.DefaultMaxPointsPerBlock && !group.hasTombstones() {


I'm slightly confused whether this is still what we want to do. We skip a group (i.e., a generation) here if it is large (sum of all files is larger than the largest permissible single file), and the first file has the default maximum points per block and there are no tombstones.

This seems to be mixing metrics from the first file in the generation (points per block) with metrics from the whole generation (combined file size). Do we need to look at the points per block of all the files in the generation? Why are we skipping a generation if it is larger than a single file can be? What's the significance of that?

I understand the original code had this strange mix of conditionals, but do we understand why, and whether we should continue with them? At the very least, the comment Skip the file if... is misleading, because we are skipping a generation which may contain more than one file, are we not?

Yes I think the comment is a bit misleading. I was mostly just keeping Plan, and PlanLevel as is... I would have no problem with modifying the existing logic in them though. Perhaps instead of checking individual file block counts and the entire group size against 2 GB I take the approach checking all the files in the group and all the block sizes in the group? Some pseudo code:

if gens <= 1 skip filesAtMaxSize = 0 filesAtMaxBlocks = 0 for file in generation if file < maxSize filesAtMaxSize++ if file.blocks < maxBlocks filesAtMaxBlocks++ if filesAtMaxSize >= 2 || filesAtMaxBlocks >= 2 || has tombstones plan

After consideration, I think you were right, @devanbenz, to change Plan and PlanLevel minimally. While their algorithms are obtuse, we shouldn't change them in the PR or at this time, to minimize the risks in what is already a large change to compaction.

tsdb/engine/tsm1/compact.go

davidby-influx · 2025-01-08T23:09:47Z

tsdb/engine/tsm1/compact_test.go

+			Size: 2048 * 1024 * 1024,
+		},
+		{
+			Path: "03-05.tsm1",


It would be good to also have this test with the small file as the first one in the generation, as well as the last.

davidby-influx · 2025-01-08T23:11:18Z

tsdb/engine/tsm1/compact_test.go

 	data := []tsm1.FileStat{
 		{
-			Path: "01-04.tsm1",
-			Size: 251 * 1024 * 1024,
+			Path: "01-05.tsm1",


Reverse the order of the file sizes, as well, to make sure the various tests of the first file in the code behave correctly whichever case it encounters (smallest first, largest first).

davidby-influx · 2025-01-08T23:14:09Z

tsdb/engine/tsm1/compact_test.go

+// under 2 GB and a mix of aggressive max blocks and default max blocks
+// it should be further compacted.
+func TestDefaultPlanner_FullyCompacted_LargeSingleGenerationUnderAggressiveBlocks(t *testing.T) {
+	// > 2 GB total group size


Test multiple orderings of the file sizes, as discussed in other comments above

davidby-influx · 2025-01-08T23:14:46Z

tsdb/engine/tsm1/compact_test.go

+func TestDefaultPlanner_FullyCompacted_LargeSingleGenerationMaxAggressiveBlocks(t *testing.T) {
+	// > 2 GB total group size
+	// 100% of files are at aggressive max block size
+	data := []tsm1.FileStat{


Test the reverse file order, as well. As discussed above.

davidby-influx · 2025-01-08T23:15:06Z

tsdb/engine/tsm1/compact_test.go

+	// > 2 GB total group size
+	// 100% of files are at aggressive max block size
+	data := []tsm1.FileStat{
+		{


Test reverse order, as well.

davidby-influx · 2025-01-08T23:15:28Z

tsdb/engine/tsm1/compact_test.go

 	data := []tsm1.FileStat{
+		{
+			Path: "01-13.tsm1",


test reverse order, as well.

davidby-influx · 2025-01-08T23:16:42Z

tsdb/engine/tsm1/compact_test.go

+		{
+			Path: "03-04.tsm1",
+			Size: 600 * 1024 * 1024,
+		},


Test reverse file order, as well.

file sizes and block counts

davidby-influx

For each test, extract the code from the creation of the DefaultPlanner to the final check of the PlanOptimize output into a named function or local lambda, taking the slice of tsm1.FileStat as an argument.

There may be commonalities between tests (the creation of the DefaultPlanner, the calling of Plan for each level, the calling of PlanLevel, and even the assertions, perhaps) which will reduce redundant code. You may be able to have one test function that does all the cases; I haven't checked.

Then, pass in the original slice of FileStats. Next, call slices.Reverse on it and test again. This gets two tests for each set of FileStats and gets rid of lots of code.

You could even have a [][]tsm1.FileStats over which you iterate, running two tests (forward and reverse) on each []tsm1.FileStat

Look here for an idea how to do this. There are others in the code base that may even be more analogous.

devanbenz · 2025-01-13T19:42:54Z

@davidby-influx I've gone ahead and moved all the cases where we plan files for compaction in to a single test case. I'm going to do the same with the tests where we do not plan them as well.

davidby-influx

Looks great. I think you may either need formatting strings in your require. functions to print the testName argument, or perhaps testing.T.Run would simplify things. If I am wrong on both of these points, LMK and I will approve as is.

davidby-influx · 2025-01-14T03:54:14Z

tsdb/engine/tsm1/compact_test.go

+
+	expectedNotFullyCompacted := func(cp *tsm1.DefaultPlanner, reasonExp string, generationCountExp int64, testName string) {
+		compacted, reason := cp.FullyCompacted()
+		require.Equal(t, reason, reasonExp, "fullyCompacted reason", testName)


Do we need a %s in the message argument to print the testName?

davidby-influx · 2025-01-14T04:00:03Z

tsdb/engine/tsm1/compact_test.go

+
+	}
+
+	for _, test := range furtherCompactedTests {


Would testing.T.Run be better here? Sometimes it can complicate things, and if it doesn't help, there's no need to switch.

Interestingly, you can see a minimal example of testing.T.Run in the testify /require documentation, or various places in our code.

For example: in the middleware

I should have mentioned this in my last review. Not a critical change, particularly if it complicates your test structure. But, it gets rid of all the testName arguments to your require. methods (which may need formatting?).

devanbenz · 2025-01-14T15:19:09Z

Looks great. I think you may either need formatting strings in your require. functions to print the testName argument, or perhaps testing.T.Run would simplify things. If I am wrong on both of these points, LMK and I will approve as is.

Ah yes, the t.Run is much cleaner. I've refactored to do it that way.

davidby-influx

One question/change. Sorry for the infinite review cycle on this one.

davidby-influx · 2025-01-14T17:07:59Z

tsdb/engine/tsm1/compact_test.go

-		cp = tsm1.NewDefaultPlanner(ffs, tsdb.DefaultCompactFullWriteColdDuration)
-		expectedFullyCompacted(cp, test.expectedFullyCompactedReasonExp, test.name)
+			// Reverse test files and re-run tests
+			slices.Reverse(test.fs)


Do we also need to reverse the block counts here?

@davidby-influx ah yes I believe so

davidby-influx

LGTM

gwossum · 2025-01-14T20:27:01Z

tsdb/engine/tsm1/compact.go

+			aggressivePointsPerBlockCount := 0
+			filesUnderMaxTsmSizeCount := 0
+			for _, tsmFile := range gens[0].files {
+				if c.FileStore.BlockCount(tsmFile.Path, 1) >= tsdb.AggressiveMaxPointsPerBlock {


I think I understand why we don't need to check the points in all blocks. Can you explain why are we checking the BlockCount for block 1 and not block 0?

NVM, figured it out. BlockIterator is a Java-style iterator, and the index is the number of times Next gets called on it, so 1 is actually the first block.

gwossum · 2025-01-14T20:35:43Z

tsdb/engine/tsm1/compact_test.go

+		expectedgenerationCount         int64
+	}
+
+	furtherCompactedTests := []PlanOptimizeTests{


Table-driven testing FTW!

gwossum · 2025-01-14T20:38:32Z

tsdb/engine/tsm1/engine.go

+						for _, f := range level4Groups[0] {
+							e.logger.Info("TSM optimized compaction on single generation running, increasing total points per block to 100_000.", zap.String("path", f))
+						}


This will be nice for determining when this is helping out, orif it is causing us issues).

gwossum · 2025-01-14T20:40:07Z

tsdb/config.go

+func SingleGenerationReason() string {
+	return fmt.Sprintf("not fully compacted and not idle because single generation with more than 2 files under %d GB and more than 1 file(s) under aggressive compaction points per block count (%d points)", int(MaxTSMFileSize/1048576000), AggressiveMaxPointsPerBlock)
+}


I won't block the PR for this, but this is still a superfluous function. The fmt.Sprintf could have been used directly with the string var.

gwossum

LGTM

devanbenz force-pushed the db/4201/compaction-bugs branch from 6e9db1b to cab638c Compare December 13, 2024 17:20

devanbenz commented Dec 16, 2024

View reviewed changes

tsdb/engine/tsm1/compact.go Outdated Show resolved Hide resolved

devanbenz force-pushed the db/4201/compaction-bugs branch from 9f5098b to 8c9d7e7 Compare December 16, 2024 18:49

devanbenz changed the title ~~feat(wip): WIP modifying compaction tests~~ feat: Modify optimized compaction to cover edge cases Dec 16, 2024

devanbenz marked this pull request as ready for review December 16, 2024 19:32

gwossum reviewed Dec 16, 2024

View reviewed changes

tsdb/engine/tsm1/compact.go Outdated Show resolved Hide resolved

gwossum reviewed Dec 16, 2024

View reviewed changes

tsdb/engine/tsm1/engine.go Outdated Show resolved Hide resolved

gwossum reviewed Dec 16, 2024

View reviewed changes

tsdb/engine/tsm1/compact.go Outdated Show resolved Hide resolved

gwossum reviewed Dec 16, 2024

View reviewed changes

tsdb/engine/tsm1/compact.go Outdated Show resolved Hide resolved

gwossum reviewed Dec 16, 2024

View reviewed changes

devanbenz force-pushed the db/4201/compaction-bugs branch from 7de2cf2 to d631314 Compare December 16, 2024 23:32

devanbenz self-assigned this Dec 16, 2024

davidby-influx requested changes Dec 17, 2024

View reviewed changes

devanbenz added 4 commits December 17, 2024 15:03

feat: Modify the PR to include optimized compaction

67849ae

for shards that may have over a 2 GB group size but many fragmented files (under 2 GB and under aggressive point per block count)

feat: Use named variables for PlanOptimize

827e859

feat: adjust test comments

5387ca3

feat: code removal from debugging

3153596

devanbenz requested review from davidby-influx and gwossum December 17, 2024 21:22

feat: setting BlockCount idx value to 1

83d28ec

davidby-influx requested changes Dec 17, 2024

View reviewed changes

devanbenz added 3 commits December 18, 2024 09:41

feat: Adjust testing and add sprintf for magic vars

f896a01

feat: need to use int64 instead of int

f15d9be

feat: touch

54c8e1c

devanbenz requested a review from davidby-influx December 18, 2024 17:08

davidby-influx requested changes Dec 18, 2024

View reviewed changes

devanbenz added 3 commits December 18, 2024 14:25

feat: Adjust tests to include lower level planning function calls

403d888

feat: Fix up some tests that I forgot to adjust

23d12e1

feat: fix typo

d3afb03

chore: rerun ci

c315b1f

devanbenz requested a review from davidby-influx December 26, 2024 19:41

davidby-influx requested changes Dec 26, 2024

View reviewed changes

tsdb/engine/tsm1/compact_test.go Outdated Show resolved Hide resolved

feat: Add a mock backfill test with mixed generations, mixed levels, …

eb0a77d

…and mixed block counts

davidby-influx previously approved these changes Dec 26, 2024

View reviewed changes

Merge branch 'master-1.x' into db/4201/compaction-bugs

1bac192

devanbenz dismissed davidby-influx’s stale review via 1bac192 January 6, 2025 18:08

feat: Fix a merge conflict where a var was renamed from fs -> fss

371f960

davidby-influx requested changes Jan 8, 2025

View reviewed changes

feat: Adding more tests reversing and mixing up some of the

5a614c4

file sizes and block counts

davidby-influx requested changes Jan 10, 2025

View reviewed changes

feat: Begin 'compacting' tests in to single test

3748c36

devanbenz added 3 commits January 13, 2025 14:01

feat: create loop for tests where there should be no further compaction

0799f00

feat: cleanup

3e69f2d

feat: Add test names to the testing struct

976291a

devanbenz requested a review from davidby-influx January 13, 2025 20:24

davidby-influx requested changes Jan 14, 2025

View reviewed changes

feat: Use t.Run instead of declaring the test name in the requires

0a2ba1e

davidby-influx requested changes Jan 14, 2025

View reviewed changes

feat: Reverse block counts

8c908c5

devanbenz requested a review from davidby-influx January 14, 2025 18:36

davidby-influx approved these changes Jan 14, 2025

View reviewed changes

gwossum reviewed Jan 14, 2025

View reviewed changes

gwossum approved these changes Jan 14, 2025

View reviewed changes

devanbenz merged commit f04105b into master-1.x Jan 14, 2025
9 checks passed

feat: Modify optimized compaction to cover edge cases #25594

feat: Modify optimized compaction to cover edge cases #25594

Conversation

devanbenz commented Nov 27, 2024 • edited Loading

gwossum left a comment

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devanbenz Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment

devanbenz commented Jan 13, 2025

davidby-influx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devanbenz commented Jan 14, 2025

davidby-influx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwossum left a comment

Choose a reason for hiding this comment

devanbenz commented Nov 27, 2024 •

edited

Loading

devanbenz Jan 9, 2025 •

edited

Loading