Unverified Commit 2ce5363f authored by Ozan Tezcan's avatar Ozan Tezcan Committed by GitHub
Browse files

Refactor higher term handling (#149)

This PR refactors code regarding `term` handling inside 
`raft_recv_appendentries()` and `raft_recv_snapshot()` without 
introducing any logic changes. 

- Deleted `raft_receive_term()` function:
    This function sets the node as follower in certain cases. Although,
    it misses the pre-candidate state.
     
    We already set the node as follower inside `raft_accept_leader()`.
    (With the correct check that also handles pre-candidate state). 

- Moved `raft_reset_transfer_leader()` into `raft_accept_leader()`. 
    Leader transfer is completed when we accept a leader. So, it makes
    sense to move it into `raft_accept_leader()`.
    
- Added some tests to verify node becomes follower even on a
     pre-candidate state when a request is received with same or
     higher term.
parent 7be57f36
......@@ -24,9 +24,8 @@ typedef enum {
RAFT_ERR_INVALID_NODEID = -8,
RAFT_ERR_LEADER_TRANSFER_IN_PROGRESS = -9,
RAFT_ERR_DONE = -10,
RAFT_ERR_STALE_TERM = -11,
RAFT_ERR_NOTFOUND = -12,
RAFT_ERR_MISUSE = -13,
RAFT_ERR_NOTFOUND = -11,
RAFT_ERR_MISUSE = -12,
} raft_error_e;
typedef enum {
......
......@@ -436,6 +436,8 @@ void raft_accept_leader(raft_server_t* me, raft_node_id_t leader)
me->timeout_elapsed = 0;
me->leader_id = leader;
raft_reset_transfer_leader(me, 0);
}
int raft_become_leader(raft_server_t *me)
......@@ -812,30 +814,6 @@ int raft_recv_appendentries_response(raft_server_t *me,
return 0;
}
static int raft_receive_term(raft_server_t* me, raft_term_t term)
{
int e;
if (raft_is_candidate(me) && me->current_term == term)
{
raft_become_follower(me);
}
else if (me->current_term < term)
{
e = raft_set_current_term(me, term);
if (0 != e)
return e;
raft_become_follower(me);
}
else if (term < me->current_term)
{
return RAFT_ERR_STALE_TERM;
}
return 0;
}
int raft_recv_appendentries(raft_server_t *me,
raft_node_t *node,
raft_appendentries_req_t *req,
......@@ -852,21 +830,22 @@ int raft_recv_appendentries(raft_server_t *me,
resp->msg_id = req->msg_id;
resp->success = 0;
e = raft_receive_term(me, req->term);
if (e != 0) {
if (e == RAFT_ERR_STALE_TERM) {
/* 1. Reply false if term < currentTerm (§5.1) */
raft_log(me, "AE term:%ld is less than current term:%ld",
req->term, me->current_term);
e = 0;
}
if (req->term < me->current_term) {
/* 1. Reply false if term < currentTerm (§5.1) */
raft_log(me, "AE term:%ld is less than current term:%ld",
req->term, me->current_term);
goto out;
}
if (req->term > me->current_term) {
e = raft_set_current_term(me, req->term);
if (e != 0) {
goto out;
}
}
/* update current leader because ae->term is up to date */
raft_accept_leader(me, req->leader_id);
raft_reset_transfer_leader(me, 0);
if (req->prev_log_idx == me->snapshot_last_idx) {
if (req->prev_log_term != me->snapshot_last_term) {
......@@ -1400,7 +1379,7 @@ int raft_recv_snapshot(raft_server_t *me,
raft_snapshot_req_t *req,
raft_snapshot_resp_t *resp)
{
int e;
int e = 0;
raft_log(me, "%d <-- %d, recv snapshot_req "
"t:%ld, lead:%d, id:%ld, si:%ld, st:%ld, o:%llu len:%llu lc:%d",
......@@ -1414,19 +1393,20 @@ int raft_recv_snapshot(raft_server_t *me,
resp->offset = 0;
resp->success = 0;
e = raft_receive_term(me, req->term);
if (e != 0) {
if (e == RAFT_ERR_STALE_TERM) {
raft_log(me, "Snapshot req term:%ld is less than current term:%ld",
req->term, me->current_term);
e = 0;
}
if (req->term < me->current_term) {
raft_log(me, "Snapshot req term:%ld is less than current term:%ld",
req->term, me->current_term);
goto out;
}
if (req->term > me->current_term) {
e = raft_set_current_term(me, req->term);
if (e != 0) {
goto out;
}
}
raft_accept_leader(me, req->leader_id);
raft_reset_transfer_leader(me, 0);
/** If we already have the snapshot or the log entries in this snapshot,
* inform the leader. */
......
......@@ -157,8 +157,6 @@ const char *raft_get_error_str(int err)
return "leader transfer is in progress";
case RAFT_ERR_DONE:
return "done";
case RAFT_ERR_STALE_TERM:
return "stale term";
case RAFT_ERR_NOTFOUND:
return "not found";
case RAFT_ERR_MISUSE:
......
......@@ -1136,6 +1136,38 @@ void TestRaft_follower_recv_appendentries_reply_false_if_term_less_than_currentt
CuAssertTrue(tc, -1 == raft_get_leader_id(r));
}
void TestRaft_follower_recv_snapshot_reply_false_if_term_less_than_currentterm(
CuTest * tc)
{
raft_cbs_t funcs = {
.persist_metadata = __raft_persist_metadata,
};
void *r = raft_new();
raft_set_callbacks(r, &funcs, NULL);
raft_add_node(r, NULL, 1, 1);
raft_add_node(r, NULL, 2, 0);
/* No leader known at this point */
CuAssertTrue(tc, -1 == raft_get_leader_id(r));
raft_snapshot_req_t req = {
.term = 1
};
/* higher current term */
raft_set_current_term(r, 5);
raft_snapshot_resp_t resp;
raft_recv_snapshot(r, raft_get_node(r, 2), &req, &resp);
CuAssertTrue(tc, resp.success == 0);
/* rejected snapshot req doesn't change the current leader. */
CuAssertTrue(tc, raft_get_leader_id(r) == RAFT_NODE_ID_NONE);
}
void TestRaft_follower_recv_appendentries_does_not_need_node(CuTest * tc)
{
raft_cbs_t funcs = {
......@@ -2351,6 +2383,67 @@ void TestRaft_candidate_recv_appendentries_from_same_term_results_in_step_down(
CuAssertIntEquals(tc, 1, raft_get_voted_for(r));
}
void TestRaft_candidate_recv_appendentries_from_higher_term_results_in_step_down(
CuTest *tc)
{
raft_appendentries_resp_t append_resp;
void *r = raft_new();
raft_add_node(r, NULL, 1, 1);
raft_add_node(r, NULL, 2, 0);
raft_set_current_term(r, 2);
raft_appendentries_req_t append_req1 = {
.term = 3,
.leader_id = 2
};
raft_set_state(r, RAFT_STATE_CANDIDATE);
raft_recv_appendentries(r, raft_get_node(r, 2), &append_req1, &append_resp);
CuAssertIntEquals(tc, 1, raft_is_follower(r));
CuAssertIntEquals(tc, 3, raft_get_current_term(r));
raft_appendentries_req_t append_req2 = {
.term = 4,
.leader_id = 2
};
raft_set_state(r, RAFT_STATE_PRECANDIDATE);
raft_recv_appendentries(r, raft_get_node(r, 2), &append_req2, &append_resp);
CuAssertIntEquals(tc, 1, raft_is_follower(r));
CuAssertIntEquals(tc, 4, raft_get_current_term(r));
}
void TestRaft_candidate_recv_snapshot_from_higher_term_results_in_step_down(
CuTest *tc)
{
raft_snapshot_resp_t snapshot_resp;
void *r = raft_new();
raft_add_node(r, NULL, 1, 1);
raft_add_node(r, NULL, 2, 0);
raft_set_current_term(r, 2);
/* Snapshot req with higher term results in step down */
raft_snapshot_req_t snapshot_req1 = {
.term = 5,
.leader_id = 2
};
raft_set_state(r, RAFT_STATE_CANDIDATE);
raft_recv_snapshot(r, raft_get_node(r, 2), &snapshot_req1, &snapshot_resp);
CuAssertIntEquals(tc, 1, raft_is_follower(r));
CuAssertIntEquals(tc, 5, raft_get_current_term(r));
raft_snapshot_req_t snapshot_req2 = {
.term = 6,
.leader_id = 2
};
raft_set_state(r, RAFT_STATE_PRECANDIDATE);
raft_recv_snapshot(r, raft_get_node(r, 2), &snapshot_req2, &snapshot_resp);
CuAssertIntEquals(tc, 1, raft_is_follower(r));
CuAssertIntEquals(tc, 6, raft_get_current_term(r));
}
void TestRaft_leader_becomes_leader_is_leader(CuTest * tc)
{
void *r = raft_new();
......@@ -5119,6 +5212,7 @@ int main(void)
SUITE_ADD_TEST(suite, TestRaft_follower_becomes_follower_is_follower);
SUITE_ADD_TEST(suite, TestRaft_follower_becomes_follower_does_not_clear_voted_for);
SUITE_ADD_TEST(suite, TestRaft_follower_recv_appendentries_reply_false_if_term_less_than_currentterm);
SUITE_ADD_TEST(suite, TestRaft_follower_recv_snapshot_reply_false_if_term_less_than_currentterm);
SUITE_ADD_TEST(suite, TestRaft_follower_recv_appendentries_does_not_need_node);
SUITE_ADD_TEST(suite, TestRaft_follower_recv_appendentries_updates_currentterm_if_term_gt_currentterm);
SUITE_ADD_TEST(suite, TestRaft_follower_recv_appendentries_does_not_log_if_no_entries_are_specified);
......@@ -5150,6 +5244,8 @@ int main(void)
SUITE_ADD_TEST(suite, TestRaft_candidate_recv_requestvote_response_becomes_follower_if_current_term_is_less_than_term);
SUITE_ADD_TEST(suite, TestRaft_candidate_recv_appendentries_frm_leader_results_in_follower);
SUITE_ADD_TEST(suite, TestRaft_candidate_recv_appendentries_from_same_term_results_in_step_down);
SUITE_ADD_TEST(suite, TestRaft_candidate_recv_appendentries_from_higher_term_results_in_step_down);
SUITE_ADD_TEST(suite, TestRaft_candidate_recv_snapshot_from_higher_term_results_in_step_down);
SUITE_ADD_TEST(suite, TestRaft_leader_becomes_leader_is_leader);
SUITE_ADD_TEST(suite, TestRaft_leader_becomes_leader_does_not_clear_voted_for);
SUITE_ADD_TEST(suite, TestRaft_leader_when_becomes_leader_all_nodes_have_nextidx_equal_to_lastlog_idx_plus_1);
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment