Ignore compaction group with only 1 block #4789
Conversation
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: yeya24 <[email protected]>
Not sure what's exactly a proper grouper. But grouper itself doesn't have to consider how many metadatas are there in a group. Because we have two use cases that needs groups:
So now the group logic is correct and we don't need to check metadata num in group itself. Just handle it in the case of compaction is good enough. |
|
Oops, sorry, brain fart. I meant that I thought the planners were taking care of this - they check whether there is any work to perform. So, this won't bring any visible change to users, right? Except for those metrics and not creating/removing dirs unnecessarily |
pkg/compact/compact_e2e_test.go
Outdated
| @@ -333,14 +333,12 @@ func testGroupCompactE2e(t *testing.T, mergeFunc storage.VerticalChunkSeriesMerg | |||
| testutil.Equals(t, 3.0, promtest.ToFloat64(grouper.compactionRunsStarted.WithLabelValues(DefaultGroupKey(metas[0].Thanos)))) | |||
| testutil.Equals(t, 3.0, promtest.ToFloat64(grouper.compactionRunsStarted.WithLabelValues(DefaultGroupKey(metas[7].Thanos)))) | |||
| // TODO(bwplotka): Looks like we do some unnecessary loops. Not a major problem but investigate. | |||
There was a problem hiding this comment.
We can probably remove these too 😄
Yeah, that's correct. There is no difference except the metrics I would say. Maybe some CPU time can be saved because we skip the planning phase. |
👍 let's remove those old comments in the test and merge this |
Signed-off-by: Ben Ye <[email protected]>
Just pushed a commit for this. Please let me know if it is good to go now. |
Signed-off-by: Ben Ye [email protected]
If the compaction group only contains 1 block metadata, then there is nothing to plan and compact. In this case, we can safely ignore the group.
Changes
Verification