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

Learner needs to respond vote requests. #58

Merged
merged 10 commits into from
May 12, 2018

Conversation

hicqu
Copy link
Contributor

@hicqu hicqu commented May 9, 2018

So that if some voters fail, rest voters and learners can still form a quorum. Fix #57 .

@hicqu
Copy link
Contributor Author

hicqu commented May 9, 2018

@BusyJay @Hoverbear @overvenus PTAL thanks.

src/raft.rs Outdated
at term {}",
self.tag,
if self.is_learner { "learner" } else { "voter" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant.

nw.peers.get_mut(&3).unwrap().tick();
}

// MsgRequeestVote should only come from 1.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many es in MsgRequeestVote

@Hoverbear
Copy link
Contributor

@hicqu I see that in #57 (comment) a candidate check was written about. In this PR I don't see an added candidate check. Was this already implemented, or found to be unnecessary?

@hicqu
Copy link
Contributor Author

hicqu commented May 10, 2018

@Hoverbear I omitted the check, because voters only send MsgRequetsVote to known voters. So if a voter receive a MsgRequestVoteResponse, it must come from a peer in the voter's voter_prs, not learner_prs.

@hicqu
Copy link
Contributor Author

hicqu commented May 10, 2018

ping @BusyJay @overvenus .

};

let mut network = Network::new(vec![Some(n1), None, Some(n3)]);
network.isolate(2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None means it won't respond to requests already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After I remove this, store 1 will be elected as leader.

let do_campaign = |nw: &mut Network| {
for _ in 0..timeout << 1 {
nw.peers.get_mut(&1).unwrap().tick();
nw.peers.get_mut(&3).unwrap().tick();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of ticking node 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To show all Vote messages come from store 1 even if 3 will also step(MsgHup).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it. Where are all those messages showed? How does it help with the test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they are still here.

}

// MsgRequestVote should only come from 1.
let msgs = read_messages(nw.peers.get_mut(&1).unwrap());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not send a MsgHup to node 1 directly?

do_campaign(&mut network);
assert_eq!(network.peers[&1].state, StateRole::Candidate);

match network.peers.get_mut(&1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwrap is OK.

@BusyJay
Copy link
Member

BusyJay commented May 11, 2018

LGTM

Copy link
Contributor

@siddontang siddontang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@BusyJay BusyJay merged commit a362370 into tikv:master May 12, 2018
@hicqu hicqu deleted the learner-vote-response branch May 14, 2018 04:17
@Hoverbear Hoverbear added this to the 0.2.0 milestone Jul 5, 2018
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.

4 participants