storage: prevent reproposals of applied commands#40505
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Sep 5, 2019
Merged
storage: prevent reproposals of applied commands#40505craig[bot] merged 1 commit intocockroachdb:masterfrom
craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Member
|
This change is |
ed9aace to
49836b2
Compare
nvb
approved these changes
Sep 5, 2019
Contributor
nvb
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/storage/replica_application_state_machine.go, line 906 at r1 (raw file):
// If any reproposals at a higher MaxLeaseIndex exist we know that they will // never successfully apply, remove them from the map to avoid future // reproposals.
Make a note of why we don't need to do this unconditionally. This will likely reference shouldRemove in replicaDecoder.retrieveLocalProposals.
pkg/storage/replica_test.go, line 11584 at r1 (raw file):
// The test does the following things: // // * Propose cmd at MaxLeaseIndex '0
'0?
pkg/storage/replica_test.go, line 11589 at r1 (raw file):
// reproposal at a higher MaxLeaseIndex. // * Simultaneously update the lease sequence number on the replica so all // future commands will fail.
With what kind of error?
TestHighestMaxLeaseIndexReproposalFinishesCommand rotted when cockroachdb#39425 was merged. Prior to that change there was an invariant that if a command was reproposed at a higher MaxLeaseIndex then the client would only be acknowledged by a command which applied at that higher MaxLeaseIndex. That change also worked to simplify context lifecycle management for replicatedCmd's by creating individual child spans for each application. This was not a complete solution however because all of the commands derived from the same proposal share a context when used for reproposals. As a consequence, commands which are reproposed and are at a higher MaxLeaseIndex than an already applied command would use a context which carried a tracing span which might already be finished. Several approaches were explored to fix this issue. The one chosen here seems to be the least invasive. The rotten test has been simplified to cover the now problematic case. The enabling mechanism for the testing is a hammer of a TestingKnob which will always refresh unconditionally all pending proposals in the proposals map at the end of a raft ready iteration. The new test fails reliably under stress in ~10s of iterations and <5s before making the change to delete proposals after they've been applied. An alternative approach would have been to partially revert cockroachdb#39425 and ensure that only commands which carry the highest MaxLeaseIndex for a proposal may be locally applied. If it's deemed cleaner and simpler then we can go with it. This approach prevents some reproposals and allows clients of commands which will fail due to non-equivalent lease changes to be acknowledged sooner of their need to retry. Fixes cockroachdb#40478 Release note: None
49836b2 to
0e81674
Compare
ajwerner
commented
Sep 5, 2019
Contributor
Author
ajwerner
left a comment
There was a problem hiding this comment.
TFTR!
bors r=nvanbenschoten
Reviewable status:
craig bot
pushed a commit
that referenced
this pull request
Sep 5, 2019
40505: storage: prevent reproposals of applied commands r=nvanbenschoten a=ajwerner TestHighestMaxLeaseIndexReproposalFinishesCommand rotted when #39425 was merged. Prior to that change there was an invariant that if a command was reproposed at a higher MaxLeaseIndex then the client would only be acknowledged by a command which applied at that higher MaxLeaseIndex. That change also worked to simplify context lifecycle management for replicatedCmd's by creating individual child spans for each application. This was not a complete solution however because all of the commands derived from the same proposal share a context when used for reproposals. As a consequence, commands which are reproposed and are at a higher MaxLeaseIndex than an already applied command would use a context which carried a tracing span which might already be finished. Several approaches were explored to fix this issue. The one chosen here seems to be the least invasive. The rotten test has been simplified to cover the now problematic case. The enabling mechanism for the testing is a hammer of a TestingKnob which will always refresh unconditionally all pending proposals in the proposals map at the end of a raft ready iteration. The new test fails reliably under stress in ~10s of iterations and <5s before making the change to delete proposals after they've been applied. An alternative approach would have been to partially revert #39425 and ensure that only commands which carry the highest MaxLeaseIndex for a proposal may be locally applied. If it's deemed cleaner and simpler then we can go with it. This approach prevents some reproposals and allows clients of commands which will fail due to non-equivalent lease changes to be acknowledged sooner of their need to retry. Fixes #40478 Release note: None Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.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.
TestHighestMaxLeaseIndexReproposalFinishesCommand rotted when #39425 was
merged. Prior to that change there was an invariant that if a command was
reproposed at a higher MaxLeaseIndex then the client would only be acknowledged
by a command which applied at that higher MaxLeaseIndex. That change also
worked to simplify context lifecycle management for replicatedCmd's by creating
individual child spans for each application. This was not a complete solution
however because all of the commands derived from the same proposal share a
context when used for reproposals. As a consequence, commands which are
reproposed and are at a higher MaxLeaseIndex than an already applied command
would use a context which carried a tracing span which might already be
finished.
Several approaches were explored to fix this issue. The one chosen here seems
to be the least invasive.
The rotten test has been simplified to cover the now problematic case. The
enabling mechanism for the testing is a hammer of a TestingKnob which will
always refresh unconditionally all pending proposals in the proposals map
at the end of a raft ready iteration. The new test fails reliably under
stress in ~10s of iterations and <5s before making the change to delete
proposals after they've been applied.
An alternative approach would have been to partially revert #39425 and
ensure that only commands which carry the highest MaxLeaseIndex for a proposal
may be locally applied. If it's deemed cleaner and simpler then we can go with
it. This approach prevents some reproposals and allows clients of commands
which will fail due to non-equivalent lease changes to be acknowledged sooner
of their need to retry.
Fixes #40478
Release note: None