-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Use LogByteSizeMergePolicy instead of TieredMergePolicy for time-based data. #92684
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
59275bb
Use LogByteSizeMergePolicy instead of TieredMergePolicy for time-base…
jpountz 81c1bca
Update docs/changelog/92684.yaml
jpountz a657bda
Add escape hatch.
jpountz 447782b
checkstyle
jpountz 2e2db78
feedback
jpountz b986142
fix tests
jpountz 6689711
Merge branch 'main' into adjacent_merges_data_streams
jpountz ae7b9ef
Adjust semantics of segments per tier.
jpountz 501943f
Fix tests.
jpountz 1757ddb
Separate configuration for merge_factor.
jpountz 3951b8a
Merge branch 'main' into adjacent_merges_data_streams
jpountz a112f4b
Decrease merge factor to 16.
jpountz 6bbcaf3
Merge branch 'main' into adjacent_merges_data_streams
jpountz 28afd5c
Fix CCR tests.
jpountz 3ca06ab
Merge branch 'main' into adjacent_merges_data_streams
jpountz 2139f08
Merge branch 'main' into adjacent_merges_data_streams
jpountz 7cd99d6
Remove the max merged segment size on time-based data.
jpountz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
pr: 92684 | ||
summary: Use `LogByteSizeMergePolicy` instead of `TieredMergePolicy` for time-based | ||
data | ||
area: Engine | ||
type: enhancement | ||
issues: [] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Should we consider raising this default "a bit" when using log-byte-size policy? To get to a similar segment size as in the tiered merge policy.
I say this because, I could imagine us wanting to merge up all the remaining small segments after rollover. And with just below 5GB, that may give 11 segs rather than 10, which seems wasteful. It would be good to have a ~5.1GB avg large segment size - to match our defaults of 50GB shards.
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.
For the record,
TieredMergePolicy
may stop merging anywhere between 2.5GB and 5GB (https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/index/TieredMergePolicy.java#L375-L390). It's less frequent than withLogByteSizeMergePolicy
because it has the ability to pack small segments together with large segments due to its ability to merge non-adjacent segments, but if one of the background merges produces a 4GB segment, then this segment won't get merged because the merge would mostly consist of rewriting this big large segment, which is wasteful.In my opinion, what we should do depends on what we think is the best practice for time-based indices:
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.
I like the 3rd option, unless if there are practical limits we could run into (heap and memory primarily). We may still want some "desired segment size" for tail merging, but that can be added as part of that effort (if we implement it). Given that it is now a factor 16, it seems very likely that with any limit we will be doing several smaller merges to fit under the indicated roof. Compared to the 5GB limit we have now, we should not see 16 of those being merged together. Correct me if I am wrong, but removing the roof is thus unlikely to lead to more actual merging than with the 5GB limit?
I suppose we might be able to ease the merging a bit with a 1GB roof. Perhaps worth trying out what the amplification would be in the two cases?
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.
There are no practical limits that I can think of and I agree that there shouldn't be more merging than what we're seeing now because 5GB is already within one mergeFactor of the rollover size of 50GB.
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.
@henningandersen Here's a video that shows the difference between max segment size = 1GB and an unbounded max segment size: https://photos.app.goo.gl/cyWZeTmthhWwC3fZ9.
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.
Sorry I forgot to mention that this video was created with 300kB flushes on average rather than 1MB.
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.
It would be good to have a new video then comparing 5GB max segment size to unbounded - with same flush size. Just to verify our mental model around the write amplification effect of the max segment size.
I wonder if we'd still want something like 100GB max segment size as protection against users using a non-default rollover size or hiccups in ILM and the rollover process.
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.
Here's a video that compares 5GB to unbounded with TieredMergePolicy: https://photos.app.goo.gl/JEDJdGFVcUAbXaKP9, and there with LogByteSizeMergePolicy (mergeFactor=16, same parameters as TieredMergePolicy otherwise): https://photos.app.goo.gl/cp3my5RzEbTHrAXSA. Very similar numbers of segments and write amplification but the bigger segments are bigger when the max segment size is unbounded. You'd need to ingest more than 50GB in an index to start seeing a bigger difference.
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.
This looks good to me, I think a 100GB (or 50GB) roof should be our new default with log byte size merge policy.
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.
I pushed a change that sets a roof of 100GB on time-based data.