Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

raft: Remove waitForCallback and centralize re-proposal logic. #3154

Merged
merged 1 commit into from
Nov 17, 2015

Conversation

bdarnell
Copy link
Contributor

When a proposal is forwarded from a follower to a leader,
waitForCallback is incorrect: what matters is whether the leader is
waiting for a pending config change callback. Instead, we need to
repropose from the follower when the leader has completed its pending
config change. This is difficult to observe directly but does have a
side effect: when a config change is dropped, it is replaced with an
empty entry. When we see such an entry, we know we have to re-propose.

Since empty entries are also used to signal leader changes, we can
remove the re-proposals in maybeSendLeaderEvent.

Fixes #2639

Review on Reviewable

@BramGruneir
Copy link
Member

LGTM

@tbg
Copy link
Member

tbg commented Nov 17, 2015

LGTM. Nice simplification.


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


multiraft/multiraft_test.go, line 579 [r1] (raw file):
s/ReProp/Reprop/?


Comments from the review on Reviewable.io

When a proposal is forwarded from a follower to a leader,
waitForCallback is incorrect: what matters is whether the *leader* is
waiting for a pending config change callback. Instead, we need to
repropose from the follower when the leader has completed its pending
config change. This is difficult to observe directly but does have a
side effect: when a config change is dropped, it is replaced with an
empty entry. When we see such an entry, we know we have to re-propose.

Since empty entries are also used to signal leader changes, we can
remove the re-proposals in maybeSendLeaderEvent.

Fixes cockroachdb#2639
@bdarnell
Copy link
Contributor Author

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


multiraft/multiraft_test.go, line 579 [r1] (raw file):
Done (and removed the hyphen after re in some other cases).


Comments from the review on Reviewable.io

bdarnell added a commit that referenced this pull request Nov 17, 2015
raft: Remove waitForCallback and centralize re-proposal logic.
@bdarnell bdarnell merged commit 396b5b2 into cockroachdb:master Nov 17, 2015
@bdarnell bdarnell deleted the raft-re-propose branch November 17, 2015 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants