Persist sequence number checkpoints #18949
Merged
Conversation
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
| currentMin = Math.min(currentMin, value); | ||
| currentMax = Math.max(currentMax, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
I cant think of any place where we would want a full top level scan like this. Can we return -1 or null or throw uoe? This acts O(n^2) in a near real time system. Do we know of any other loops like this in this stats code?
There was a problem hiding this comment.
This implementation will not be the permanent implementation, it is temporary as we iterate in this feature branch. This method is only used when starting an engine, and is needed for recovery (either locally or from a peer).
This commit adds persistence for local and global sequence number checkpoints. We also recover the max sequence number in a shard, although there will be loss here today from delete operations. This will be addressed in a follow-up.
| mergeScheduler = scheduler = new EngineMergeScheduler(engineConfig.getShardId(), engineConfig.getIndexSettings()); | ||
| throttle = new IndexThrottle(); | ||
| this.searcherFactory = new SearchFactory(logger, isClosed, engineConfig); | ||
| try { | ||
| writer = createWriter(openMode == EngineConfig.OpenMode.CREATE_INDEX_AND_TRANSLOG); | ||
| final long localCheckpoint = loadLocalCheckpointFromCommit(writer); |
There was a problem hiding this comment.
Nit: maybe load a SeqNoStats with one method? this is probably how it's going to be?
|
looks good! left some minor comments |
This commit adds a nocommit to the SeqNoFieldType#stats method as the implementation is temporary until late-binding commits are available.
This commit moves the global checkpoint commit key from the global checkpoint service to the internal engine where it is better placed for clarity. This commit also enables marking this field as having default access.
This commit moves the local checkpoint commit key from the local checkpoint service to the internal engine where it is better placed for clarity. This commit also enables marking this field as having default access.
This commit removes a constructor from the global checkpoint service that was only used in tests.
This commit removes a constructor from the local checkpoint service that was only used in tests.
This commit reduces the visibility of certain fields and methods in the local and global checkpoint services.
This commit simplifies the loading of sequence number stats from a commit point by combining all the logic for loading into a single method that returns an instance of SeqNoStats rather than three separate methods that return longs.
This commit fixes an issue with the calculation of sequence number field stats when all docs are deleted. Namely, when all docs are deleted we would return a FieldStats instance when the intention is to return null.
This commit clarifies local and global checkpoint service initialization by adding Javadocs and rewriting some of the field initialization to clarify the initial values.
This commit simplifies an internal engine test for sequence number checkpoint persistence. For now, we will ignore the issues surrounding delete which will be addressed in follow-up work. We also remove the complexity of the interaction between a primary shard and a replica (although we simulate advancing the local checkpoint on a replica and the corresponding advancement of the global checkpoint). Finally, we add a translog recovery component to the test.
This commit adds a test for global checkpoint persistence to the commit stats internal engine test. Note that this test now randomizes the values of the local and global checkpoints.
@bleskes I've responded to all of your feedback. |
|
LGTM |
64 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This commit adds persistence for local and global sequence number
checkpoints. We also recover the max sequence number in a shard,
although there will be loss here today from delete operations. This will
be addressed in a follow-up.
Relates #10708