storage: clean up below-Raft Split and Merge locking#39158
Merged
craig[bot] merged 3 commits intocockroachdb:masterfrom Jul 30, 2019
Merged
storage: clean up below-Raft Split and Merge locking#39158craig[bot] merged 3 commits intocockroachdb:masterfrom
craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
If this operation fails then we can't simply return an error and not apply the command. We need to be deterministic at this point. The only option we have is to fatal. See cockroachdb#19448 (comment). The commit also removes the crash reporting added here, which doesn't appear to have ever fired after two years. Release note: None
…anup This was being used to detect failed Splits, but it doesn't look like this is actually possible any more. In terms of the code, `rResult.Split` should never be nil unless a Raft command is being rejected below Raft, in which case we will never have called `acquireSplitLock` in the first place. In terms of the comment, it sounds like this is protecting against failures that were only relevant before proposer evaluated KV. Release note: None
…t lock The comment was added in 0dd7d1a, before proposer evaluated KV. Reproposals are not an issue because of the max lease index, which ensures that only a single instance of a command applies. Replays of the Range split transaction are also not an issue because they are protected against above Raft. Release note: None
Member
|
This change is |
bdarnell
approved these changes
Jul 30, 2019
Contributor
bdarnell
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 3 of 3 files at r2, 1 of 1 files at r3.
Reviewable status:complete! 0 of 0 LGTMs obtained
Contributor
Author
|
bors r=bdarnell |
craig bot
pushed a commit
that referenced
this pull request
Jul 30, 2019
39158: storage: clean up below-Raft Split and Merge locking r=bdarnell a=nvanbenschoten This PR contains three related cleanups that I stumbled into while cleaning up raft entry application in preparation for #38954. The first cleanup is that we avoid improperly handling errors that may be returned when acquiring the split/merge locks. If this operation fails then we can't simply return an error and not apply the command. We need to be deterministic at this point. The only option we have is to fatal. See #19448 (comment). The second cleanup is that we then remove stale code that was attempting to recover from failed split and merge application. This error handling only made sense in a pre-proposer evaluated KV world. The third commit addresses a TODO to assert that the RHS range during a split is not initialized. The TODO looks like it went in before proposer evaluated KV, which protects us against both replays and reproposals. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Contributor
Build succeeded |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 PR contains three related cleanups that I stumbled into while cleaning up raft entry application in preparation for #38954.
The first cleanup is that we avoid improperly handling errors that may be returned when acquiring the split/merge locks. If this operation fails then we can't simply return an error and not apply the command. We need to be deterministic at this point. The only option we have is to fatal. See #19448 (comment).
The second cleanup is that we then remove stale code that was attempting to recover from failed split and merge application. This error handling only made sense in a pre-proposer evaluated KV world.
The third commit addresses a TODO to assert that the RHS range during a split is not initialized. The TODO looks like it went in before proposer evaluated KV, which protects us against both replays and reproposals.