fix deadlock during prevote migration process #42

csmoe · 2018-03-12T12:23:23Z

Fixes #20

siddontang · 2018-03-13T02:09:26Z

tests/cases/test_raft.rs

+ // n1.state == Leader
+ // n2.state == Follower
+ // n3.state == Candidate
+ if nt.peers[&1].state != StateRole::Leader {


you can use assert_eq here

siddontang · 2018-03-13T02:09:40Z

tests/cases/test_raft.rs

+ // n1.Term == 2
+ // n2.Term == 2
+ // n3.Term == 4
+ if nt.peers[&1].term != 2 {


siddontang · 2018-03-13T02:10:18Z

tests/cases/test_raft.rs

+ // n2 is follower with term 2
+ // n3 is pre-candidate with term 4, and less log
+
+ // simulate leader down


please format your code with rustfmt nightly version 0.3.4

BusyJay · 2018-03-19T12:50:02Z

src/raft.rs

@@ -955,6 +955,27 @@ impl<T: Storage> Raft<T> {
 // will not create disruptive term increases
 let to_send = new_message(m.get_from(), MessageType::MsgAppendResponse, None);


Missing a comment.

Can you clarify what you'd like to see?

maybe need to copy from etcd?

BusyJay · 2018-03-19T13:05:14Z

tests/cases/test_raft.rs

+ n2.become_follower(1, INVALID_ID);
+ n3.become_follower(1, INVALID_ID);
+
+ // We intentionally do not enable pre_vote for n3, this is done so in order


The comment should be put to L3854.

BusyJay · 2018-03-19T13:06:58Z

tests/cases/test_raft.rs

+ let mut e = Entry::new();
+ e.set_data(b"some data".to_vec());
+ nt.send(vec![
+ new_message_with_entries(1, 1, MessageType::MsgPropose, vec![e])


You can use new_message(1, 1, MessageType::MsgPropose, 1).

BusyJay · 2018-03-19T13:09:01Z

tests/cases/test_raft.rs

+ // n2.state == Follower
+ // n3.state == Candidate
+ assert_eq!(nt.peers[&1].state,
+ StateRole::Leader, "node 1 state: {:?}, want {:?}",


You don't have to customize the message as assert_eq did it for you already.

BusyJay · 2018-03-19T13:09:59Z

tests/cases/test_raft.rs

+ // version cluster with replicas with pre_vote enabled, and replicas without.
+ let mut nt = Network::new(vec![Some(n1), Some(n2), Some(n3)]);
+
+ nt.peers.get_mut(&2).unwrap().pre_vote = true;


You can use new_test_raft_with_pre_vote.

BusyJay · 2018-03-19T13:11:02Z

tests/cases/test_raft.rs

+fn test_prevote_migration_can_complete_election() {
+ let mut nt = new_prevote_migration_cluster();
+
+ // n1 is leader with term 2


Put the comment to L3923.

BusyJay · 2018-03-19T13:18:45Z

tests/cases/test_raft.rs

+
+ // Do we have a leader?
+ // assert_eq!(nt.peers[&2].state, StateRole::Leader )
+ if nt.peers[&2].state != StateRole::Leader && nt.peers[&3].state != StateRole::Follower {


Use assert! instead.

BusyJay · 2018-03-19T13:39:43Z

src/raft.rs

@@ -955,6 +955,27 @@ impl<T: Storage> Raft<T> {
 // will not create disruptive term increases
 let to_send = new_message(m.get_from(), MessageType::MsgAppendResponse, None);
 self.send(to_send);
+ } else if m.get_msg_type() == MessageType::MsgRequestPreVote {
+ // Before pre_vote enable, there may have candidate with higher term,


The comment is confusing. I thought candidate here refers to the one who sends prevote request. Turns out it refers to the receiver. Could you make it better?

Hoverbear · 2018-04-12T22:38:02Z

Hi @csmoe, would you like any help with getting this PR merged? I see @BusyJay left a few comments. If you're busy I can wrap things up for you.

csmoe · 2018-04-12T23:40:52Z

@Hoverbear sorry for a delayed commit, I totally forgot this thread.
Feel free to do anything, thank you.

Hoverbear · 2018-04-22T05:40:51Z

@BusyJay Can you give this another look regarding the feedback you gave?

siddontang · 2018-04-23T00:51:49Z

PTAL @hicqu

siddontang · 2018-05-01T23:49:06Z

tests/cases/test_raft.rs

+ nt.send(vec![new_message(2, 2, MessageType::MsgHup, 0)]);
+
+ // Do we have a leader?
+ assert_eq!(nt.peers[&2].state, StateRole::Leader);


can we guarantee the leader can always be elected? It is different from https://github.com/coreos/etcd/pull/8525/files#diff-ad12a9c9b66e1507ca04edc74b4b1e8aR3386

siddontang · 2018-05-01T23:53:23Z

Rest LGTM

siddontang · 2018-05-07T20:24:25Z

any update @csmoe?

siddontang · 2018-05-08T07:32:45Z

tests/cases/test_raft.rs

+ nt.send(vec![new_message(2, 2, MessageType::MsgHup, 0)]);
+
+ // Do we have a leader?
+ assert_eq!((nt.peers[&2].state, nt.peers[&3].state), (StateRole::Leader, StateRole::Follower));


I think this is still different from https://github.com/coreos/etcd/pull/8525/files?utf8=%E2%9C%93&diff=unified#diff-ad12a9c9b66e1507ca04edc74b4b1e8aR3386.

sorry, I don't know how to fix this since I thought this tuple comparsion might work like && here. could you give me some tips?
Will simulating leader down(nt.solate(1)) before new election guarantee that?

in your test, you require 2 must be the leader, and 3 must be the follower. But in the etcd test, if 2 is the leader, 3 can be not the follower.

oh, thank you, I made a logic mistake here.

BusyJay

rest LGTM

BusyJay · 2018-05-09T15:21:57Z

src/raft.rs

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

 pub check_quorum: bool,
- pre_vote: bool,
+ pub pre_vote: bool,


Please add a doc hidden attribute.

BusyJay · 2018-05-09T15:44:21Z

tests/cases/test_raft.rs

+ nt.isolate(1);
+
+ // Call for elections from both n2 and n3.
+ nt.send(vec![new_message(2, 2, MessageType::MsgHup, 0)]);


Order is important, although it won't affect the test result (yet).

BusyJay · 2018-05-09T15:45:16Z

tests/cases/test_raft.rs

+ nt.send(vec![new_message(3, 3, MessageType::MsgHup, 0)]);
+
+ assert_eq!(nt.peers[&1].state,
+ StateRole::Leader, "node 1 state: {:?}, want {:?}",


Debug message is unnecessary.

siddontang

LGTM

siddontang · 2018-05-10T18:01:18Z

PTAL @Hoverbear

Hoverbear

LGTM. 👍

Hoverbear · 2018-05-11T02:47:28Z

Please run cargo fmt and we can merge this. :) We just merged #55 which runs code on the CI through rustfmt.

csmoe · 2018-05-11T03:28:41Z

@Hoverbear The current rustfmt in CI is stable-ver, but:

Warning: can't set `use_try_shorthand = true`, unstable features are only available in nightly channel.

So rustfmt config might need changes.

siddontang · 2018-05-14T09:45:15Z

Ping @Hoverbear

Hoverbear · 2018-05-14T21:12:34Z

Opened #62 to reflect the cargo fmt warnings.

siddontang reviewed Mar 13, 2018

View reviewed changes

tests/cases/test_raft.rs Outdated

// n1.Term == 2

// n2.Term == 2

// n3.Term == 4

if nt.peers[&1].term != 2 {

Copy link

Contributor

siddontang Mar 13, 2018

Choose a reason for hiding this comment

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

ditto

siddontang reviewed Mar 13, 2018

View reviewed changes

BusyJay reviewed Mar 19, 2018

View reviewed changes

Hoverbear force-pushed the fix_deadlock_during_prevote_migration_process branch from 9f9cfb4 to 2c24c57 Compare April 22, 2018 05:40

Hoverbear requested a review from BusyJay April 22, 2018 05:40

siddontang reviewed May 1, 2018

View reviewed changes

siddontang reviewed May 8, 2018

View reviewed changes

BusyJay reviewed May 9, 2018

View reviewed changes

siddontang previously approved these changes May 10, 2018

View reviewed changes

Hoverbear previously approved these changes May 11, 2018

View reviewed changes

csmoe and others added 7 commits May 11, 2018 11:24

fix deadlock during prevote migration process

3012d3c

fix formatting and if -> assert

28219d5

Reflect feedback

6004119

make sure the leader is elected.

424816a

fix logic

a32ce62

add doc_hidden and fix test

1080163

rebase && format

c74e0af

csmoe dismissed stale reviews from Hoverbear and siddontang via c74e0af May 11, 2018 03:27

csmoe force-pushed the fix_deadlock_during_prevote_migration_process branch from fa74996 to c74e0af Compare May 11, 2018 03:27

Merge branch 'master' into fix_deadlock_during_prevote_migration_process

7dd78e9

Hoverbear approved these changes May 14, 2018

View reviewed changes

Hoverbear merged commit 444e99e into tikv:master May 14, 2018

csmoe deleted the fix_deadlock_during_prevote_migration_process branch May 14, 2018 23:47

Hoverbear added this to the 0.3.0 milestone Jul 5, 2018

		@@ -955,6 +955,27 @@ impl<T: Storage> Raft<T> {
		// will not create disruptive term increases
		let to_send = new_message(m.get_from(), MessageType::MsgAppendResponse, None);

fix deadlock during prevote migration process #42

fix deadlock during prevote migration process #42

Conversation

csmoe commented Mar 12, 2018

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

Hoverbear commented Apr 12, 2018

csmoe commented Apr 12, 2018

Hoverbear commented Apr 22, 2018

siddontang commented Apr 23, 2018

Choose a reason for hiding this comment

siddontang commented May 1, 2018

siddontang commented May 7, 2018

Choose a reason for hiding this comment

csmoe May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csmoe May 9, 2018 • edited Loading

Choose a reason for hiding this comment

BusyJay left a 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 left a comment

Choose a reason for hiding this comment

siddontang commented May 10, 2018

Hoverbear left a comment

Choose a reason for hiding this comment

Hoverbear commented May 11, 2018

csmoe commented May 11, 2018

siddontang commented May 14, 2018

Hoverbear commented May 14, 2018

csmoe May 8, 2018 •

edited

Loading

csmoe May 9, 2018 •

edited

Loading