-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Introduce translog generation rolling #23606
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
Merged
Merged
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
cabd994
Introduce translog generation folding
jasontedor 152cae7
Fix ordering of checking/locks
jasontedor 91d5f85
Remove unnecessary import
jasontedor c88e788
Change method to be instance method
jasontedor 7e69ae8
Remove generation parameter from fold
jasontedor fb00dd3
Remove obsolete catch block
jasontedor 0d5e6e2
Move field
jasontedor bbab126
Rename folding generation to rolling generation
jasontedor 421b336
Merge branch 'master' into translog-generation
jasontedor 8f6b609
Stricter roll checking
jasontedor 70ade35
Merge branch 'master' into translog-generation
jasontedor d9fbc42
Add test for rolling and committing
jasontedor 9076d79
Simplify tests
jasontedor b79053e
Fix typo
jasontedor b4ed67d
Merge branch 'master' into translog-generation
jasontedor 1669224
Stronger test for precommit/roll/commit sequences
jasontedor 35d0119
Add Javadocs to IndexSettings#getGenerationThresholdSize
jasontedor 32afd9e
Add assert message
jasontedor 518f12e
Merge branch 'master' into translog-generation
jasontedor 7480faf
Merge branch 'master' into translog-generation
jasontedor dd14ba8
Migrate translog generation rolling to index shard
jasontedor f59233c
Add Javadocs and tidy up
jasontedor 3c78802
Merge branch 'master' into translog-generation
jasontedor 192a7ce
Funny business
jasontedor 6c2adb6
Revert "Funny business"
jasontedor 1a31061
Committed generation
jasontedor 07fc092
Remove unneeded field
jasontedor 2e7f722
Fix thread pool name
jasontedor a14937d
Change method name
jasontedor 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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.
nit: wondering if this should just be
index.translog.generation_sizeThere 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 picked
index.translog.generation_threshold_sizeto be consistent withindex.translog.flush_threshold_size. Do you still wonder if it should be changed?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.
We can keep as is then. I don't mind much.
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.
can you maybe document what happens if that size is exceeded or add a link to the explain?
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 wanna understand why this is
64MBwhy can't we useINDEX_TRANSLOG_FLUSH_THRESHOLD_SIZE_SETTINGor it's default instead rather than an arbitrary value? I would love to get some insight that we also can document? Then there is also the question why we can't simply flush this from the outside viaIndexShard#maybeFlush, that would simplify the translog potentially, one property that I liked about theTranslog#add()operation was that it would never acquire a write lock. We are now entering potentially dangerous territory here, locking can be a beast.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.
Jason can say why he chose 64MB - I think any value we choose now will be arbitrary so I was good with it for now and tweak later if needed.
We want a number that's much smaller than the flush size. We're heading towards keeping the generations that are needed to recover all seq# ops after a certain point. That means that we won't always clean previous generation when flushing. If we use the flush size for generations we can end up in a poisonous where we repeatedly try to flush but the translog is not trimmed.
Do you mean doing both flushing and potentially opening a new generation from the same method that is called after indexing? i.e., call it maybeFlushAndRoll ( :) ) ? I would be good with that (though prefer the current approach).
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 was looking into having a
Translog#rollover()method we can from themaybeFlushwe can renamemaybeFlushtoonAfterOpterationto not necessarily yield impl details. I am a bit concerned about the write lock, which might not be a problem today but maybe tomorrow.makes sense. lets document this. it's not clear from reviewing the PR
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 dd14ba8 to move the control of rolling to index shard. Would @nik9000, @bleskes, and @s1monw please take another look?