Skip to content
New issue

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

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

Already on GitHub? # to your account

raft: become_pre_candidate resets votes #83

Merged
merged 4 commits into from
Jul 4, 2018
Merged

Conversation

Hoverbear
Copy link
Contributor

Currently we don't reset the votes in become_pre_candidate when etcd does.

This PR resolves this.

@Hoverbear Hoverbear added the Question A question to be answered. label Jun 30, 2018
@Hoverbear Hoverbear self-assigned this Jun 30, 2018
siddontang
siddontang previously approved these changes Jun 30, 2018
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

@huachaohuang
Copy link

LGTM

@BusyJay
Copy link
Member

BusyJay commented Jun 30, 2018

Please also port the tests from etcd-io/etcd#8346

@siddontang
Copy link
Contributor

ping @Hoverbear

@Hoverbear
Copy link
Contributor Author

@BusyJay PTAL. :)

);

// check state
assert_eq!(
Copy link
Contributor

Choose a reason for hiding this comment

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

why Candidate here? not PreCandidate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// election in next round.
#[test]
fn test_prevote_with_split_vote() {
let bootstrap = |id| {
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 use new_test_raft_with_prevote?

// check whether the term values are expected
assert_eq!(
network.peers[&2].term, 3,
"peer 2 term: {:?}, want {:?}",
Copy link
Member

Choose a reason for hiding this comment

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

The default error message is quite informative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I will change it here, just changed it in #84.

@Hoverbear Hoverbear merged commit 2f8ed16 into master Jul 4, 2018
@Hoverbear Hoverbear deleted the reset-pre-candidate branch July 4, 2018 17:26
@Hoverbear Hoverbear added this to the 0.3.1 milestone Jul 5, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Question A question to be answered.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants