raft: require app to consume result from Ready() #10920
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
I changed `(*RawNode).Ready`'s behavior in etcd-io#10892 in a problematic way. Previously, `Ready()` would create and immediately "accept" a Ready (i.e. commit the app to actually handling it). In etcd-io#10892, Ready() became a pure read-only operation and the "accepting" was moved to `Advance(rd)`. As a result it was illegal to use the RawNode in certain ways while the Ready was being handled. Failure to do so would result in dropped messages (and perhaps worse). For example, with the following operations 1. `rd := rawNode.Ready()` 2. `rawNode.Step(someMsg)` 3. `rawNode.Advance(rd)` `someMsg` would be dropped, because `Advance()` would clear out the outgoing messages thinking that they had all been handled by the client. I mistakenly assumed that this restriction had existed prior, but this is incorrect. I noticed this while trying to pick up the above PR in CockroachDB, where it caused unit test failures, precisely due to the above example. This PR reestablishes the previous behavior (result of `Ready()` must be handled by the app) and adds a regression test. While I was there, I carried out a few small clarifying refactors.
Codecov Report
@@ Coverage Diff @@
## master #10920 +/- ##
=========================================
+ Coverage 62.79% 62.99% +0.2%
=========================================
Files 400 400
Lines 37689 37687 -2
=========================================
+ Hits 23665 23741 +76
+ Misses 12415 12345 -70
+ Partials 1609 1601 -8
Continue to review full report at Codecov.
|
bdarnell
approved these changes
Jul 24, 2019
|
Thanks @bdarnell. |
absolute8511
added a commit
to youzan/ZanRedisDB
that referenced
this pull request
Jul 8, 2021
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.
I changed
(*RawNode).Ready's behavior in #10892 in a problematic way.Previously,
Ready()would create and immediately "accept" a Ready(i.e. commit the app to actually handling it). In #10892, Ready() became
a pure read-only operation and the "accepting" was moved to
Advance(rd). As a result it was illegal to use the RawNode in certainways while the Ready was being handled. Failure to do so would result in
dropped messages (and perhaps worse). For example, with the following
operations
rd := rawNode.Ready()rawNode.Step(someMsg)rawNode.Advance(rd)someMsgwould be dropped, becauseAdvance()would clear out theoutgoing messages thinking that they had all been handled by the client.
I mistakenly assumed that this restriction had existed prior, but this
is incorrect.
I noticed this while trying to pick up the above PR in CockroachDB,
where it caused unit test failures, precisely due to the above example.
This PR reestablishes the previous behavior (result of
Ready()mustbe handled by the app) and adds a regression test.
While I was there, I carried out a few small clarifying refactors.
Touches cockroachdb/cockroach#39046.