Move poll & quorum functionality into ProgressSet #121
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
Hoverbear
changed the title
Hoverbear
force-pushed
the
move-poll-quorum
branch
6 times, most recently
from
9a8703a to
1d3f19f
Compare
Hoverbear
force-pushed
the
move-poll-quorum
branch
3 times, most recently
from
ec5f6e6 to
ad34bf0
Compare
Hoverbear
force-pushed
the
move-poll-quorum
branch
3 times, most recently
from
b56b13b to
0676de3
Compare
Hoverbear
force-pushed
the
move-poll-quorum
branch
from
0676de3 to
942ab2d
Compare
hicqu
reviewed
Sep 29, 2018
hicqu
reviewed
Sep 29, 2018
hicqu
reviewed
Sep 29, 2018
hicqu
reviewed
Sep 29, 2018
|
@huachaohuang sorry, wrong person. ^^ |
Hoverbear
commented
Oct 30, 2018
This comment has been minimized.
This comment has been minimized.
|
@overvenus @hicqu please take a look again. |
hicqu
previously approved these changes
Nov 8, 2018
|
LGTM. 👍 Please take look at |
overvenus
reviewed
Nov 8, 2018
|
@hicqu @overvenus Please consider the responses to your feedback, I have suggested new names. :) |
|
@overvenus thanks for the great input. :) Reflected your suggestion. |
hicqu
approved these changes
Nov 21, 2018
|
LGTM. |
|
bors r+ |
bors bot
added a commit
that referenced
this pull request
Nov 21, 2018
121: Move poll & quorum functionality into ProgressSet r=Hoverbear a=Hoverbear Based off #119. **You are seeing that change set as well here, so go review that, then this after merging it! :)** Migrate poll and quorum related functionality inside `ProgressSet`. These functions operated on the `ProgressSet` anyways, they were just scoped to the `Raft`. # Rationale With the introduction of Joint Consensus, the idea of a Quorum is no longer always a single value. (If the cluster is in a joint state it actually has two quorums. The old configuration and the new one.) In the interest of isolating responsibility and dulling sharp edges, this PR migrates code the following functionality: * `check_quorum_active`: Migrated, it was doing a mutable fold on the peers and resetting their recent active, then returning if there was enough for quorum. The `ProgressSet` can do this itself, and it means we can avoid having to return some enum of either `(quorum)` or `(new_quorum, old_quorum)`. The `Raft` module need not care. * `poll`: Was split into a mutating vote-setter function, and a check function (non mutating). The check function returns a semantically meaningful enum now. This was a good idea since `poll(vote) >= quorum`, while still valid, may be comparing tuples or values. Internalizing this logic and instead returning a semantic value helps with understanding. * `minimum_committed_index`: Since in a joint consensus state we need to check the minimum committed index of both old and new, we can internalize this check and just return a bool. # Benefits This PR essentially amounts to API cleaning. It doesn't change any behavior, but it's a change we should do to make future work easier. # Future Work When Joint Consensus (#101 ) lands it will change these new functions to be joint-aware and correctly return values regardless of the configuration state. Co-authored-by: Hoverbear <[email protected]> Co-authored-by: Hoverbear <[email protected]> Co-authored-by: A. Hobden <[email protected]> Co-authored-by: A. Hobden <[email protected]> Co-authored-by: qupeng <[email protected]>
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.
Based off #119. You are seeing that change set as well here, so go review that, then this after merging it! :)
Migrate poll and quorum related functionality inside
ProgressSet.These functions operated on the
ProgressSetanyways, they were just scoped to theRaft.Rationale
With the introduction of Joint Consensus, the idea of a Quorum is no longer always a single value. (If the cluster is in a joint state it actually has two quorums. The old configuration and the new one.)
In the interest of isolating responsibility and dulling sharp edges, this PR migrates code the following functionality:
check_quorum_active: Migrated, it was doing a mutable fold on the peers and resetting their recent active, then returning if there was enough for quorum. TheProgressSetcan do this itself, and it means we can avoid having to return some enum of either(quorum)or(new_quorum, old_quorum). TheRaftmodule need not care.poll: Was split into a mutating vote-setter function, and a check function (non mutating). The check function returns a semantically meaningful enum now. This was a good idea sincepoll(vote) >= quorum, while still valid, may be comparing tuples or values. Internalizing this logic and instead returning a semantic value helps with understanding.minimum_committed_index: Since in a joint consensus state we need to check the minimum committed index of both old and new, we can internalize this check and just return a bool.Benefits
This PR essentially amounts to API cleaning. It doesn't change any behavior, but it's a change we should do to make future work easier.
Future Work
When Joint Consensus (#101 ) lands it will change these new functions to be joint-aware and correctly return values regardless of the configuration state.