Fix for #2593 #3074
Merged
Fix for #2593 #3074
+18
−99
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
|
LGTM. Seems reasonable to me that testing for this is Hard™. |
| // itself is removed, the node is re-added to the group | ||
| // (which will trigger another re-proposal that will succeed), or the | ||
| // proposal is committed. | ||
| if initialProposal { |
There was a problem hiding this comment.
isn't initialProposal equal to the ok in _, ok := g.pending[p.commandID] and thus not required?
|
LGTM mod my question. |
|
LGTM mod @tschottdorf's question (I think he's right) |
added 2 commits
November 10, 2015 12:45
The cause of the issue occurred as such: + Incoming client request to a node creates a trace context. + The context is attached to a raft command which is proposed. The command is added to the 'pending' map in multiraft before being proposed. The client request will be answered once the proposed command is committed and applied. + Concurrently, another raft command changes the configuration of the range's raft group, removing this node's replica. In existing code, all pending commands on that node which target that replica are synchronously dismissed with an error; the trace is therefore finalized. + However, while the replica has been removed from the group, the group itself has not yet been removed from the node; the proposed command can actually commit, it just commits after the configuration change. + When the committed change is applied, it attempts to use the trace, but the trace has already been finalized. The fix is to no longer abort pending commands on a replica just because that replica has been removed from the group; it is not yet safe to immediately abort pending requests, because they may actually complete. Instead, we do not abort commands until the group itself is removed (by the range GC queue).
Replica.Quiesce() was added in an attempt to fix cockroachdb#2593; however, it did not fix that issue, and was not necessary to fix it in the first place. This commit removes Replica.Quiesce().
mrtracy
force-pushed
the
mtracy/fix_finalized_trace
branch
from
bd39581 to
30953c1
Compare
|
Yes, you are correct that initialProposal was not necessary. I have updated this with Tobi's suggestion, everything appears to still be in working order. |
|
|
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.
Finally got to the bottom of this, with special thanks to Tobias, Tamir and Ben.
The test which exercises this most consistently is TestStoreRangeRebalance; however, it was only able to quickly reproduce this in the multicpu branch, where it was reproduced with reliability.
A six node cluster was able to run stably under load for well over an hour with this fix; the bug appears to be completely gone.