From 4636cf6ddeb51c3143843c7e4c7608be2e8e2fe4 Mon Sep 17 00:00:00 2001 From: qupeng Date: Wed, 9 May 2018 16:04:31 +0800 Subject: [PATCH 1/7] Learner needs to respond vote requests. --- src/raft.rs | 28 ++++----------- tests/cases/test_raft.rs | 76 +++++++++++++++++++--------------------- 2 files changed, 43 insertions(+), 61 deletions(-) diff --git a/src/raft.rs b/src/raft.rs index e5ea4a865..c5e275cd0 100644 --- a/src/raft.rs +++ b/src/raft.rs @@ -27,16 +27,16 @@ use std::cmp; -use rand::{self, Rng}; use eraftpb::{Entry, EntryType, HardState, Message, MessageType, Snapshot}; use fxhash::FxHashMap; use protobuf::repeated::RepeatedField; +use rand::{self, Rng}; -use super::storage::Storage; -use super::progress::{Inflights, Progress, ProgressSet, ProgressState}; use super::errors::{Error, Result, StorageError}; +use super::progress::{Inflights, Progress, ProgressSet, ProgressState}; use super::raft_log::{self, RaftLog}; use super::read_only::{ReadOnly, ReadOnlyOption, ReadState}; +use super::storage::Storage; // CAMPAIGN_PRE_ELECTION represents the first phase of a normal election when // Config.pre_vote is true. @@ -828,6 +828,7 @@ impl Raft { return; } + // Only send vote request to voters. let prs = self.take_prs(); prs.voters() .keys() @@ -1016,24 +1017,6 @@ impl Raft { debug!("{} ignoring MsgHup because already leader", self.tag); }, MessageType::MsgRequestVote | MessageType::MsgRequestPreVote => { - if self.is_learner { - // TODO: learner may need to vote, in case of node down when confchange. - info!( - "{} [logterm: {}, index: {}, vote: {}] ignored {:?} from {} \ - [logterm: {}, index: {}] at term {}: learner can not vote", - self.tag, - self.raft_log.last_term(), - self.raft_log.last_index(), - self.vote, - m.get_msg_type(), - m.get_from(), - m.get_log_term(), - m.get_index(), - self.term, - ); - return Ok(()); - } - // We can vote if this is a repeat of a vote we've already cast... let can_vote = (self.vote == m.get_from()) || // ...we haven't voted and we don't think there's a leader yet in this term... @@ -1083,9 +1066,10 @@ impl Raft { fn log_vote_approve(&self, m: &Message) { info!( - "{} [logterm: {}, index: {}, vote: {}] cast {:?} for {} [logterm: {}, index: {}] \ + "{}({}) [logterm: {}, index: {}, vote: {}] cast {:?} for {} [logterm: {}, index: {}] \ at term {}", self.tag, + if self.is_learner { "learner" } else { "voter" }, self.raft_log.last_term(), self.raft_log.last_index(), self.vote, diff --git a/tests/cases/test_raft.rs b/tests/cases/test_raft.rs index 0cf4b74a8..9f12edc70 100644 --- a/tests/cases/test_raft.rs +++ b/tests/cases/test_raft.rs @@ -25,10 +25,10 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::cmp; use std::collections::HashMap; use std::ops::Deref; use std::ops::DerefMut; -use std::cmp; use std::panic::{self, AssertUnwindSafe}; use protobuf::{self, RepeatedField}; @@ -36,8 +36,8 @@ use raft::eraftpb::{ConfChange, ConfChangeType, ConfState, Entry, EntryType, Har MessageType, Snapshot}; use rand; -use raft::*; use raft::storage::MemStorage; +use raft::*; pub fn ltoa(raft_log: &RaftLog) -> String { let mut s = format!("committed: {}\n", raft_log.committed); @@ -897,34 +897,22 @@ fn test_vote_from_any_state_for_type(vt: MessageType) { StateRole::Follower ); assert_eq!( - r.term, - new_term, + r.term, new_term, "{:?},{:?}, term {}, want {}", - vt, - state, - r.term, - new_term + vt, state, r.term, new_term ); assert_eq!(r.vote, 2, "{:?},{:?}, vote {}, want 2", vt, state, r.vote); } else { // In a pre-vote, nothing changes. assert_eq!( - r.state, - state, + r.state, state, "{:?},{:?}, state {:?}, want {:?}", - vt, - state, - r.state, - state + vt, state, r.state, state ); assert_eq!( - r.term, - orig_term, + r.term, orig_term, "{:?},{:?}, term {}, want {}", - vt, - state, - r.term, - orig_term + vt, state, r.term, orig_term ); // If state == Follower or PreCandidate, r hasn't voted yet. // In Candidate or Leader, it's voted for itself. @@ -2030,11 +2018,9 @@ fn test_candidate_reset_term(message_type: MessageType) { // follower c term is reset with leader's assert_eq!( - nt.peers[&3].term, - nt.peers[&1].term, + nt.peers[&3].term, nt.peers[&1].term, "follower term expected same term as leader's {}, got {}", - nt.peers[&1].term, - nt.peers[&3].term, + nt.peers[&1].term, nt.peers[&3].term, ) } @@ -3798,21 +3784,6 @@ fn test_learner_promotion() { assert_eq!(network.peers[&2].state, StateRole::Leader); } -// TestLearnerCannotVote checks that a learner can't vote even it receives a valid Vote request. -#[test] -fn test_learner_cannot_vote() { - let mut n2 = new_test_learner_raft(2, vec![1], vec![2], 10, 1, new_storage()); - n2.become_follower(1, INVALID_ID); - - let mut msg_vote = new_message(1, 2, MessageType::MsgRequestVote, 0); - msg_vote.set_term(2); - msg_vote.set_log_term(11); - msg_vote.set_index(11); - n2.step(msg_vote).unwrap(); - - assert_eq!(n2.msgs.len(), 0); -} - // TestLearnerLogReplication tests that a learner can receive entries from the leader. #[test] fn test_learner_log_replication() { @@ -3978,3 +3949,30 @@ fn test_remove_learner() { assert!(n1.prs().nodes().is_empty()); assert!(n1.prs().learner_nodes().is_empty()); } + +#[test] +fn test_learner_respond_vote() { + let mut n1 = new_test_learner_raft(1, vec![1, 2, 3], vec![], 10, 1, new_storage()); + n1.become_follower(1, INVALID_ID); + n1.reset_randomized_election_timeout(); + + let mut n3 = new_test_learner_raft(2, vec![1, 2], vec![3], 10, 1, new_storage()); + n3.become_follower(1, INVALID_ID); + n3.reset_randomized_election_timeout(); + + let timeout = n1.get_election_timeout(); + + let mut network = Network::new(vec![Some(n1), None, Some(n3)]); + for _ in 0..timeout << 1 { + network.peers.get_mut(&1).unwrap().tick(); + network.peers.get_mut(&3).unwrap().tick(); + } + + // MsgRequeestVote should only come from 1. + let msgs = read_messages(network.peers.get_mut(&1).unwrap()); + msgs.iter().for_each(|m| assert_eq!(m.get_from(), 1)); + + // Learner can respond vote messages so that 1 will be leader. + network.send(msgs); + assert_eq!(network.peers[&1].state, StateRole::Leader); +} From d2c8ff54adc3de9ea4d72f750cfb037c719d5d12 Mon Sep 17 00:00:00 2001 From: qupeng Date: Wed, 9 May 2018 18:47:47 +0800 Subject: [PATCH 2/7] improve test case. --- tests/cases/test_raft.rs | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/tests/cases/test_raft.rs b/tests/cases/test_raft.rs index 9f12edc70..20a947d22 100644 --- a/tests/cases/test_raft.rs +++ b/tests/cases/test_raft.rs @@ -3952,27 +3952,44 @@ fn test_remove_learner() { #[test] fn test_learner_respond_vote() { - let mut n1 = new_test_learner_raft(1, vec![1, 2, 3], vec![], 10, 1, new_storage()); + let mut n1 = new_test_learner_raft(1, vec![1, 2], vec![3], 10, 1, new_storage()); n1.become_follower(1, INVALID_ID); n1.reset_randomized_election_timeout(); - let mut n3 = new_test_learner_raft(2, vec![1, 2], vec![3], 10, 1, new_storage()); + let mut n3 = new_test_learner_raft(3, vec![1, 2], vec![3], 10, 1, new_storage()); n3.become_follower(1, INVALID_ID); n3.reset_randomized_election_timeout(); let timeout = n1.get_election_timeout(); + 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(); + } + + // MsgRequeestVote should only come from 1. + let msgs = read_messages(nw.peers.get_mut(&1).unwrap()); + msgs.iter().for_each(|m| assert_eq!(m.get_from(), 1)); + nw.send(msgs); + }; + let mut network = Network::new(vec![Some(n1), None, Some(n3)]); - for _ in 0..timeout << 1 { - network.peers.get_mut(&1).unwrap().tick(); - network.peers.get_mut(&3).unwrap().tick(); - } + network.isolate(2); + + // Can't elect new leader because 1 won't send MsgRequestVote to 3. + do_campaign(&mut network); + assert_eq!(network.peers[&1].state, StateRole::Candidate); - // MsgRequeestVote should only come from 1. - let msgs = read_messages(network.peers.get_mut(&1).unwrap()); - msgs.iter().for_each(|m| assert_eq!(m.get_from(), 1)); + match network.peers.get_mut(&1) { + Some(raft) => { + raft.add_node(3); + raft.become_follower(1, INVALID_ID); + } + None => unreachable!(), + } - // Learner can respond vote messages so that 1 will be leader. - network.send(msgs); + // After promote 3 to voter, election should success. + do_campaign(&mut network); assert_eq!(network.peers[&1].state, StateRole::Leader); } From b35556de24984f39139d353df4b22a750863f816 Mon Sep 17 00:00:00 2001 From: qupeng Date: Thu, 10 May 2018 12:17:12 +0800 Subject: [PATCH 3/7] address comments. --- src/raft.rs | 3 +-- tests/cases/test_raft.rs | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/raft.rs b/src/raft.rs index c5e275cd0..65e0bae31 100644 --- a/src/raft.rs +++ b/src/raft.rs @@ -1066,10 +1066,9 @@ impl Raft { fn log_vote_approve(&self, m: &Message) { info!( - "{}({}) [logterm: {}, index: {}, vote: {}] cast {:?} for {} [logterm: {}, index: {}] \ + "{} [logterm: {}, index: {}, vote: {}] cast {:?} for {} [logterm: {}, index: {}] \ at term {}", self.tag, - if self.is_learner { "learner" } else { "voter" }, self.raft_log.last_term(), self.raft_log.last_index(), self.vote, diff --git a/tests/cases/test_raft.rs b/tests/cases/test_raft.rs index 20a947d22..327ecae13 100644 --- a/tests/cases/test_raft.rs +++ b/tests/cases/test_raft.rs @@ -3968,7 +3968,7 @@ fn test_learner_respond_vote() { nw.peers.get_mut(&3).unwrap().tick(); } - // MsgRequeestVote should only come from 1. + // MsgRequestVote should only come from 1. let msgs = read_messages(nw.peers.get_mut(&1).unwrap()); msgs.iter().for_each(|m| assert_eq!(m.get_from(), 1)); nw.send(msgs); From b14dc5fedb61596ba1a1cad87875e28b2a64fe11 Mon Sep 17 00:00:00 2001 From: qupeng Date: Thu, 10 May 2018 21:12:04 +0800 Subject: [PATCH 4/7] address comments. --- tests/cases/test_raft.rs | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/tests/cases/test_raft.rs b/tests/cases/test_raft.rs index 327ecae13..fc81e5c14 100644 --- a/tests/cases/test_raft.rs +++ b/tests/cases/test_raft.rs @@ -3967,11 +3967,8 @@ fn test_learner_respond_vote() { nw.peers.get_mut(&1).unwrap().tick(); nw.peers.get_mut(&3).unwrap().tick(); } - - // MsgRequestVote should only come from 1. - let msgs = read_messages(nw.peers.get_mut(&1).unwrap()); - msgs.iter().for_each(|m| assert_eq!(m.get_from(), 1)); - nw.send(msgs); + let msg = new_message(1, 1, MessageType::MsgHup, 0); + nw.send(vec![msg]); }; let mut network = Network::new(vec![Some(n1), None, Some(n3)]); @@ -3981,15 +3978,8 @@ fn test_learner_respond_vote() { do_campaign(&mut network); assert_eq!(network.peers[&1].state, StateRole::Candidate); - match network.peers.get_mut(&1) { - Some(raft) => { - raft.add_node(3); - raft.become_follower(1, INVALID_ID); - } - None => unreachable!(), - } - // After promote 3 to voter, election should success. + network.peers.get_mut(&1).unwrap().add_node(3); do_campaign(&mut network); assert_eq!(network.peers[&1].state, StateRole::Leader); } From 9b99942901551aa18a91af43b0d12eff599622df Mon Sep 17 00:00:00 2001 From: qupeng Date: Thu, 10 May 2018 22:11:40 +0800 Subject: [PATCH 5/7] address comments. --- tests/cases/test_raft.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/cases/test_raft.rs b/tests/cases/test_raft.rs index fc81e5c14..2a2febe33 100644 --- a/tests/cases/test_raft.rs +++ b/tests/cases/test_raft.rs @@ -3965,7 +3965,6 @@ fn test_learner_respond_vote() { 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(); } let msg = new_message(1, 1, MessageType::MsgHup, 0); nw.send(vec![msg]); From 06f898f7a2234891211a1b6bce960287791d9701 Mon Sep 17 00:00:00 2001 From: qupeng Date: Fri, 11 May 2018 13:56:09 +0800 Subject: [PATCH 6/7] don't need to any tick more with MsgHup. --- tests/cases/test_raft.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/cases/test_raft.rs b/tests/cases/test_raft.rs index 2a2febe33..cc1d2e344 100644 --- a/tests/cases/test_raft.rs +++ b/tests/cases/test_raft.rs @@ -3960,12 +3960,7 @@ fn test_learner_respond_vote() { n3.become_follower(1, INVALID_ID); n3.reset_randomized_election_timeout(); - let timeout = n1.get_election_timeout(); - let do_campaign = |nw: &mut Network| { - for _ in 0..timeout << 1 { - nw.peers.get_mut(&1).unwrap().tick(); - } let msg = new_message(1, 1, MessageType::MsgHup, 0); nw.send(vec![msg]); }; From faa583cbbeb3f7e6d62f967fa1962e79a603e39a Mon Sep 17 00:00:00 2001 From: qupeng Date: Fri, 11 May 2018 14:32:58 +0800 Subject: [PATCH 7/7] update protobuf dependency version. --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 7cd09e289..7d04b1bbc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,7 +13,7 @@ categories = ["algorithms", "database-implementations"] [dependencies] log = "0.4.1" -protobuf = "1.2" +protobuf = "~1.5" quick-error = "1.2.1" rand = "0.4" fxhash = "0.2.1"