diff --git a/src/communication/raft/raft-inl.hpp b/src/communication/raft/raft-inl.hpp index 7b8850222..9ab55e278 100644 --- a/src/communication/raft/raft-inl.hpp +++ b/src/communication/raft/raft-inl.hpp @@ -197,6 +197,9 @@ bool RaftMemberImpl::SendRPC(const std::string &recipient, return false; } + DCHECK(reply.Term() >= term_) + << "Response term should be >= request term"; + /* [Raft thesis, Section 3.3] * "Current terms are exchanged whenever servers communicate; if one server's * current term is smaller than the other's, then it updates its current term @@ -424,6 +427,18 @@ PeerRPCReply::RequestVote RaftMemberImpl::OnRequestVote( return reply; } + /* [Raft thesis, Section 3.3] + * "Current terms are exchanged whenever servers communicate; if one server's + * current term is smaller than the other's, then it updates its current term + * to the larger value. If a candidate or leader discovers that its term is + * out of date, it immediately reverts to follower state." */ + if (request.candidate_term > term_) { + if (mode_ != RaftMode::FOLLOWER) { + CandidateOrLeaderTransitionToFollower(); + } + UpdateTermAndVotedFor(request.candidate_term, {}); + } + /* [Raft thesis, Section 3.6.1] * "Raft uses the voting process to prevent a candidate from winning an * election unless its log contains all committed entries. (...) The @@ -457,18 +472,7 @@ PeerRPCReply::RequestVote RaftMemberImpl::OnRequestVote( return reply; } - /* [Raft thesis, Section 3.3] - * "Current terms are exchanged whenever servers communicate; if one server's - * current term is smaller than the other's, then it updates its current term - * to the larger value. If a candidate or leader discovers that its term is - * out of date, it immediately reverts to follower state." */ - if (request.candidate_term > term_) { - if (mode_ != RaftMode::FOLLOWER) { - CandidateOrLeaderTransitionToFollower(); - } - } - - /* Now we now we will vote for this candidate, because it's term is at least + /* Now we know we will vote for this candidate, because it's term is at least * as big as ours and we haven't voted for anyone else. */ UpdateTermAndVotedFor(request.candidate_term, request.candidate_id); diff --git a/tests/unit/raft.cpp b/tests/unit/raft.cpp index c18edefa7..6f15df7e9 100644 --- a/tests/unit/raft.cpp +++ b/tests/unit/raft.cpp @@ -344,12 +344,11 @@ TEST_P(OnRequestVoteTest, RequestVoteTest) { EXPECT_EQ(reply.vote_granted, GetParam().expected_reply); + /* Our term should always be at least as large as sender's term. */ /* If we accepted the request, our term should be equal to candidate's term * and voted_for should be set. */ - EXPECT_EQ(reply.term, - reply.vote_granted ? GetParam().peer_term : GetParam().term); - EXPECT_EQ(storage_.term_, - reply.vote_granted ? GetParam().peer_term : GetParam().term); + EXPECT_EQ(reply.term, std::max(GetParam().peer_term, GetParam().term)); + EXPECT_EQ(storage_.term_, std::max(GetParam().peer_term, GetParam().term)); EXPECT_EQ(storage_.voted_for_, reply.vote_granted ? "b" : GetParam().voted_for); } @@ -444,7 +443,6 @@ struct OnAppendEntriesTestParam { LogIndex peer_next_index; bool expected_reply; - TermId expected_term; std::vector> expected_log; }; @@ -468,9 +466,10 @@ TEST_P(OnAppendEntriesTest, All) { {GetParam().peer_term, "b", last_log_index, last_log_term, entries, 0}); EXPECT_EQ(reply.success, GetParam().expected_reply); - EXPECT_EQ(reply.term, GetParam().expected_term); + EXPECT_EQ(reply.term, std::max(GetParam().peer_term, GetParam().term)); EXPECT_EQ(storage_.log_, GetParam().expected_log); } + /* Member 'a' recieved AppendEntries RPC from member 'b'. The request will * contain no log entries, representing just a heartbeat, as it is not * important in these scenarios. */ @@ -484,7 +483,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {2}, {3}, {4}, {5}, {5}, {6}}, 7, false, - 8, {{1}, {1}, {2}}}, /* we're missing entries 4, 5 and 6 -> decline, but update term */ OnAppendEntriesTestParam{4, @@ -493,7 +491,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {2}, {3}, {4}, {5}, {5}, {6}}, 7, false, - 8, {{1}, {1}, {2}}}, /* we're missing entry 4 -> decline, but update term */ OnAppendEntriesTestParam{5, @@ -502,7 +499,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {2}, {3}, {4}, {5}, {5}, {6}}, 5, false, - 8, {{1}, {1}, {2}}}, /* log terms don't match at entry 4 -> decline, but update term */ OnAppendEntriesTestParam{5, @@ -511,7 +507,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {3}, {3}, {4}, {5}, {5}, {6}}, 4, false, - 8, {{1}, {1}, {2}}}, /* logs match -> accept and update term */ OnAppendEntriesTestParam{5, @@ -520,7 +515,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {2}, {3}, {4}, {5}, {5}, {6}}, 4, true, - 8, {{1}, {1}, {2}, {3}, {4}, {5}, {5}, {6}}}, /* now follow some log truncation tests */ /* no truncation, append a single entry */ @@ -531,7 +525,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}, 9, true, - 8, {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}}, /* no truncation, append multiple entries */ OnAppendEntriesTestParam{ @@ -541,7 +534,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}, 4, true, - 8, {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}}, /* no truncation, leader's log is prefix of ours */ OnAppendEntriesTestParam{ @@ -551,7 +543,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}, 4, true, - 8, {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}, {6}}}, /* another one, now with entries from newer term */ OnAppendEntriesTestParam{ @@ -561,7 +552,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}, 4, true, - 8, {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}, {7}, {7}}}, /* no truncation, partial match between our log and appended entries */ @@ -572,7 +562,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}, 4, true, - 8, {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}}, /* truncate suffix */ OnAppendEntriesTestParam{ @@ -582,7 +571,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}, 5, true, - 8, {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}}, /* truncate suffix, with partial match between our log and appened entries */ @@ -593,7 +581,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}, 4, true, - 8, {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}}, /* delete whole log */ OnAppendEntriesTestParam{ @@ -603,7 +590,6 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}, 1, true, - 8, {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}}, /* append on empty log */ OnAppendEntriesTestParam{ @@ -613,5 +599,4 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}, 1, true, - 8, {{1}, {1}, {1}, {4}, {4}, {5}, {5}, {6}, {6}, {6}}}));