-
Notifications
You must be signed in to change notification settings - Fork 9.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow compact series merger to be configurable #8836
Conversation
Signed-off-by: yeya24 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, LGTM, thanks!
I think it's simple and straightforward to merge but will give few hours for other maintainers to look on this.
@@ -323,7 +323,7 @@ func TestBlockSize(t *testing.T) { | |||
require.NoError(t, err) | |||
require.Equal(t, expAfterDelete, actAfterDelete, "after a delete reported block size doesn't match actual disk size") | |||
|
|||
c, err := NewLeveledCompactor(context.Background(), nil, log.NewNopLogger(), []int64{0}, nil) | |||
c, err := NewLeveledCompactor(context.Background(), nil, log.NewNopLogger(), []int64{0}, nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point, struct config would do better... 🤔 or ``With...` Options. Do you mind @yeya24 adding quick issue for this? 🤗
set = storage.NewMergeChunkSeriesSet(sets, storage.NewCompactingChunkSeriesMerger(storage.ChainedSeriesMerge)) | ||
// Merge series using specified chunk series merger. | ||
// The default one is the compacting series merger. | ||
set = storage.NewMergeChunkSeriesSet(sets, c.mergeFunc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uu, we actually saved couple of allocs here (:
cc @pstibrany @pracucci to gather cortex feedback & provide awareness |
Thank you! Cortex currently uses existing vertical compaction and deduplication support, which works fine for our case. I don't quite understand the use case here – I tried to read thanos-io/thanos#1014, but haven't found what other deduplication algorithm will be used. But this change – adding support for different algorithms – looks good to me. |
It's being used here: thanos-io/thanos#4239 (comment) (: |
Signed-off-by: yeya24 [email protected]
This pr allows custom compact series merger. Now the compact series merger is a simple 1:1 deduplication series merger.
In thanos there is one use case of offline deduplication and we want to use other deduplication algorithms, see thanos-io/thanos#1014.
This pr updates
NewLeveledCompactor
constructor with one more argument to specify the merger. The default value is the original compacting series merger.