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
25 changes: 4 additions & 21 deletions src/raft.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -828,6 +828,7 @@ impl<T: Storage> Raft<T> {
return;
}

// Only send vote request to voters.
let prs = self.take_prs();
prs.voters()
.keys()
Expand Down Expand Up @@ -1016,24 +1017,6 @@ impl<T: Storage> Raft<T> {
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...
Expand Down
83 changes: 44 additions & 39 deletions tests/cases/test_raft.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,19 @@
// 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};
use raft::eraftpb::{ConfChange, ConfChangeType, ConfState, Entry, EntryType, HardState, Message,
MessageType, Snapshot};
use rand;

use raft::*;
use raft::storage::MemStorage;
use raft::*;

pub fn ltoa(raft_log: &RaftLog<MemStorage>) -> String {
let mut s = format!("committed: {}\n", raft_log.committed);
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
)
}

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -3978,3 +3949,37 @@ 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], vec![3], 10, 1, new_storage());
n1.become_follower(1, INVALID_ID);
n1.reset_randomized_election_timeout();

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();
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.

}
let msg = new_message(1, 1, MessageType::MsgHup, 0);
nw.send(vec![msg]);
};

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.


// 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);

// 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);
}