raft: port pre-vote feature #1444

hhkbp2 · 2016-12-26T06:46:06Z

Hi,

This PR port the pre-vote feature for etcd/raft.
It merges #1330, #1425, and etcd-io/etcd#7060.

hhkbp2 · 2016-12-26T06:46:41Z

PTAL @siddontang @BusyJay @ngaut

hhkbp2 · 2016-12-27T09:36:38Z

PTAL @siddontang @BusyJay @ngaut

siddontang · 2016-12-27T14:17:59Z

src/raft/raft.rs

+            if m.get_term() == 0 {
+                // Pre-vote RPCs are sent at a term other than our actual term, so the code
+                // that sends these messages is responsible for setting the term.
+                panic!("term should be set when sending {:?}", m.get_msg_type());


siddontang · 2016-12-27T14:20:15Z

src/raft/raft.rs

@@ -624,6 +662,16 @@ impl<T: Storage> Raft<T> {
        info!("{} became candidate at term {}", self.tag, self.term);
    }

+    pub fn become_pre_candidate(&mut self) {
+        assert!(self.state != StateRole::Leader,


The usage of assert! is consistent with become_candidate().

siddontang · 2016-12-27T14:24:57Z

tests/test_raft.rs

-fn ents(terms: Vec<u64>) -> Interface {
+// ents_with_config creates a raft state machine with a sequence of log entries at
+// the given terms.
+fn ents_with_config(terms: Vec<u64>, pre_vote: bool) -> Interface {


where is the config?

pre_vote is the config.

siddontang · 2016-12-28T01:41:37Z

src/raft/raft.rs

+                               self.raft_log.committed + 1,
+                               raft_log::NO_LIMIT)
+                        .expect("unexpected error getting unapplied entries");
+                    let n = self.num_pending_conf(&ents[..]);


seem that &ents is enough?

siddontang · 2016-12-28T01:58:06Z

src/raft/raft.rs

+                    if n != 0 && self.raft_log.committed > self.raft_log.applied {
+                        warn!("{} cannot campaign at term {} since there are still {} pending \
+                               configuration changes to apply",
+                              self.id,


siddontang · 2016-12-28T02:07:47Z

PTAL @BusyJay

siddontang · 2016-12-28T07:52:36Z

tests/test_raft.rs

+        }
+        let base = ltoa(raft_log);
+        let l = ltoa(&nt.peers[&(1 + i as u64)].raft_log);
+        if base != l {


not the same with etcd :

base := ltoa(tt.raftLog) if sm, ok := nt.peers[1+uint64(i)].(*raft); ok { l := ltoa(sm.raftLog) if g := diffu(base, l); g != "" { t.Errorf("#%d: diff:\n%s", i, g) } } else { t.Logf("#%d: empty log", i) }

This piece of code is to ensure two logs have the same applied index, commit index, and log entries. In TiKV, it simply use String::ne instead of diffu in every test case.

siddontang · 2016-12-28T07:56:23Z

tests/test_raft.rs

@@ -1179,6 +1500,15 @@ fn test_msg_append_response_wait_reset() {

 #[test]
 fn test_recv_msg_request_vote() {


etcd has only one Vote test here:

func TestRecvMsgVote(t *testing.T) { testRecvMsgVote(t, pb.MsgVote) } func testRecvMsgVote(t *testing.T, msgType pb.MessageType) {

No prevote test.

It's deleted in etcd-io/etcd#6975

etcd-io/etcd#6975 will be taken in this PR later.

BusyJay · 2016-12-28T07:31:41Z

src/raft/raft.rs

@@ -190,6 +202,7 @@ pub struct Raft<T: Storage> {
    heartbeat_elapsed: usize,

    pub check_quorum: bool,
+    pub pre_vote: bool,


BusyJay · 2016-12-28T07:35:08Z

src/raft/raft.rs

+            // proposals are a way to forward to the leader and
+            // should be treated as local message.
+            if m.get_msg_type() != MessageType::MsgPropose &&
+               m.get_msg_type() != MessageType::MsgReadIndex {


Why check ReadIndex?

MsgReadIndex is also forwarded to leader.

BusyJay · 2016-12-28T07:52:55Z

src/raft/raft.rs

+                if (self.vote == INVALID_ID || m.get_term() > self.term ||
+                    self.vote == m.get_from()) &&
+                   self.raft_log.is_up_to_date(m.get_index(), m.get_log_term()) {
+                    info!("{} [logterm: {}, index: {}, vote: {}] cast {:?} for {} [logterm: {}, \


Reuse the function log_vote_approve and log_vote_reject instead.

The log message is used once. It's simple to log all arguments in-place.

The function is too long, it's more readable to keep these two line consuming statements apart.

BusyJay · 2016-12-28T08:19:24Z

tests/test_raft.rs

+
+        // three logs futher along than 0, but in the same term so rejection
+        // are returned instead of the votes being ignored.
+        (Network::new_with_config(vec![None,


Missing a case.

The case of "log converge" is moved outside, see #1425.

BusyJay · 2016-12-28T08:24:18Z

tests/test_raft.rs

-fn ents(terms: Vec<u64>) -> Interface {
+// ents_with_config creates a raft state machine with a sequence of log entries at
+// the given terms.
+fn ents_with_config(terms: Vec<u64>, pre_vote: bool) -> Interface {


Why change this?

Add pre_vote.

BusyJay · 2016-12-28T08:34:03Z

tests/test_raft.rs

+    nt.send(vec![new_message(3, 3, MessageType::MsgHup, 0)]);
+    let wlog = new_raft_log(vec![empty_entry(1, 1)], 2, 1);
+    let wlog2 = new_raft_log_with_storage(new_storage());
+    let tests = vec![


I think it's more clear to add the index to case suit than using enumerate.

BusyJay · 2016-12-28T08:37:35Z

tests/test_raft.rs

        (StateRole::Candidate, 3, 3, 1, true),
    ];

    for (j, (state, i, term, vote_for, w_reject)) in tests.drain(..).enumerate() {
        let raft_log = new_raft_log(vec![empty_entry(2, 1), empty_entry(2, 2)], 3, 0);
        let mut sm = new_test_raft(1, vec![1], 10, 1, new_storage());
+        if msg_type == MessageType::MsgRequestPreVote {


Why set this?

BusyJay · 2016-12-28T08:38:18Z

tests/test_raft.rs

        sm.state = state;
        sm.vote = vote_for;
        sm.raft_log = raft_log;
-        let mut m = new_message(2, 0, MessageType::MsgRequestVote, 0);
+        let mut m = new_message(2, 0, msg_type, 0);


I don't think it's 0.

What value it should be?

BusyJay · 2016-12-28T08:41:12Z

tests/test_raft.rs

@@ -529,41 +556,275 @@ fn test_progress_paused() {
 }

 #[test]
-fn test_leader_election() {
+fn test_leader_election_no_pre_vote() {


I think it's better to keep the original name and rename test_leader_election to test_leader_election_with_config.

keep the same name with etcd TestLeaderElection

BusyJay · 2016-12-28T08:44:24Z

tests/test_raft.rs

    let mut r = new_test_raft(1, vec![1], 5, 1, new_storage());
+    r.pre_vote = pre_vote;


I think it's better to add a new function new_raft. Set it after initialization assumes that pre_vote doesn't need any initialization work.

hhkbp2 · 2016-12-30T04:10:31Z

The modification for raft in etcd-io/etcd#6975 is merged.
PTAL @siddontang @BusyJay

hhkbp2 · 2017-01-03T02:18:40Z

PTAL @siddontang @BusyJay

siddontang · 2017-01-03T15:18:26Z

LGTM

hhkbp2 · 2017-01-04T02:39:52Z

PTAL @BusyJay @zhangjinpeng1987

BusyJay

rest LGTM. By the way, it will be more clear to port a pr at a time.

BusyJay · 2017-01-04T07:22:07Z

tests/test_raft.rs

-        let raft = &network.peers[&1];
-        if raft.state != state {
-            panic!("#{}: state = {:?}, want {:?}", i, raft.state, state);
+        let raft = network.peers.get(&1).unwrap();


Use &network.peers[&1]; instead.

hhkbp2 · 2017-01-05T03:24:06Z

PTAL @BusyJay

BusyJay

LGTM

mrmiywj and others added 20 commits December 20, 2016 20:15

raft-prevote

142bd47

format

2a57f38

update cargo.locl

29c5117

update

fd9689c

update due to clippy

3e1f738

make format

5564d45

update

d4b4425

update

ee8dc23

update due to jay's comments

bdc63c9

updaet cargo.toml

bf417a2

update

89ba690

use expect instead of unwrap

94b9584

update due to hhkbp2

97ab381

update

e1f5e87

make format

2777ce4

s/precandidate/pre_candidate/

06dfb2a

add comments

77e687a

revise impl

9af8f20

port fix for leader election log converge test case

0edb8bf

clean up codes

d75b2c3

hhkbp2 changed the title ~~[DNM] raft: port pre-vote feature~~ raft: port pre-vote feature Dec 27, 2016

siddontang reviewed Dec 27, 2016

View reviewed changes

siddontang reviewed Dec 28, 2016

View reviewed changes

address comment

f09e5c1

siddontang reviewed Dec 28, 2016

View reviewed changes

BusyJay reviewed Dec 28, 2016

View reviewed changes

hhkbp2 added 2 commits December 30, 2016 11:13

address comment

2c9a277

address comment

c4a806a

address comment

a6c7130

BusyJay reviewed Jan 4, 2017

View reviewed changes

address comment

beeb659

Merge branch 'master' into raft-prevote

f1cfc70

BusyJay approved these changes Jan 5, 2017

View reviewed changes

Merge branch 'master' into raft-prevote

5eaf8a1

hhkbp2 merged commit 609afb0 into master Jan 6, 2017

hhkbp2 deleted the raft-prevote branch January 6, 2017 03:44

hhkbp2 mentioned this pull request Jan 9, 2017

raft: Fix election "logs converge" test #1419

Closed

This was referenced May 31, 2018

Introduce Prevote tikv/raft-rs#39

Closed

Enable Prevote #3133

Merged

sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019

		@@ -1179,6 +1500,15 @@ fn test_msg_append_response_wait_reset() {

		#[test]
		fn test_recv_msg_request_vote() {

		let mut r = new_test_raft(1, vec![1], 5, 1, new_storage());
		r.pre_vote = pre_vote;

raft: port pre-vote feature #1444

raft: port pre-vote feature #1444

Conversation

hhkbp2 commented Dec 26, 2016

hhkbp2 commented Dec 26, 2016

hhkbp2 commented Dec 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented Dec 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BusyJay Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhkbp2 commented Dec 30, 2016

hhkbp2 commented Jan 3, 2017

siddontang commented Jan 3, 2017

hhkbp2 commented Jan 4, 2017

BusyJay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhkbp2 commented Jan 5, 2017

BusyJay left a comment

Choose a reason for hiding this comment

BusyJay Dec 30, 2016 •

edited

Loading