Unverified Commit 8db9930c authored by Ozan Tezcan's avatar Ozan Tezcan Committed by GitHub
Browse files

Handle response error cases gracefully (#137)

A node can receive responses from followers who are not
aware of they have been removed from the cluster configuration.
In this case, recv_ family functions can be called without 
a node structure. We should ignore these messages.

We should also ignore snapshot and appendentries responses
if we are not leader anymore. Returning an error code for 
this case does not make sense because application cannot 
do anything but ignore the error code. 

This PR changes return value to zero for these cases. 
parent 703e70c2
......@@ -708,12 +708,8 @@ int raft_recv_appendentries_response(raft_server_t *me,
raft_get_nodeid(me), raft_node_get_id(node),
resp->msg_id, resp->term, resp->success, resp->current_idx);
if (!node) {
return -1;
}
if (!raft_is_leader(me)) {
return RAFT_ERR_NOT_LEADER;
if (!node || !raft_is_leader(me)) {
return 0;
}
if (resp->msg_id < raft_node_get_match_msgid(node) ||
......@@ -1508,8 +1504,8 @@ int raft_recv_snapshot_response(raft_server_t *me,
raft_get_nodeid(me), raft_node_get_id(node), resp->msg_id,
resp->term, resp->success, resp->offset, resp->last_chunk);
if (!raft_is_leader(me)) {
return RAFT_ERR_NOT_LEADER;
if (!node || !raft_is_leader(me)) {
return 0;
}
if (resp->term < me->current_term ||
......
......@@ -3185,18 +3185,40 @@ void TestRaft_leader_recv_appendentries_response_retry_only_if_leader(CuTest * t
aer.term = 1;
aer.success = 1;
aer.current_idx = 1;
CuAssertTrue(tc, RAFT_ERR_NOT_LEADER == raft_recv_appendentries_response(r, raft_get_node(r, 2), &aer));
CuAssertTrue(tc, 0 == raft_recv_appendentries_response(r, raft_get_node(r, 2), &aer));
CuAssertTrue(tc, NULL == sender_poll_msg_data(sender));
}
void TestRaft_leader_recv_appendentries_response_without_node_fails(CuTest * tc)
void TestRaft_leader_recv_appendentries_response_without_node(CuTest *tc)
{
raft_cbs_t funcs = {
.persist_metadata = __raft_persist_metadata,
/* Receiving appendentries_resp without a node should be ignored silently.*/
void *r = raft_new();
raft_add_node(r, NULL, 1, 1);
raft_add_node(r, NULL, 2, 0);
raft_add_node(r, NULL, 3, 0);
/* I'm the leader */
raft_set_state(r, RAFT_STATE_LEADER);
raft_set_current_term(r, 1);
/* receive mock success responses */
raft_appendentries_resp_t aer = {
.term = 5000,
.success = 1,
.current_idx = 0
};
CuAssertIntEquals(tc, 0, raft_recv_appendentries_response(r, NULL, &aer));
/* Verify response was ignored and term is not 5000. */
CuAssertIntEquals(tc, 1, raft_get_current_term(r));
}
void TestRaft_leader_recv_snapshot_response_without_node(CuTest *tc)
{
/* Receiving snapshot_resp without a node should be ignored silently. */
void *r = raft_new();
raft_set_callbacks(r, &funcs, NULL);
raft_add_node(r, NULL, 1, 1);
raft_add_node(r, NULL, 2, 0);
......@@ -3207,12 +3229,17 @@ void TestRaft_leader_recv_appendentries_response_without_node_fails(CuTest * tc)
raft_set_current_term(r, 1);
/* receive mock success responses */
raft_appendentries_resp_t aer;
memset(&aer, 0, sizeof(raft_appendentries_resp_t));
aer.term = 1;
aer.success = 1;
aer.current_idx = 0;
CuAssertIntEquals(tc, -1, raft_recv_appendentries_response(r, NULL, &aer));
raft_snapshot_resp_t resp = {
.term = 5000,
.success = 1,
.msg_id = 1,
.offset = 0,
.last_chunk = 0,
};
CuAssertIntEquals(tc, 0, raft_recv_snapshot_response(r, NULL, &resp));
/* Verify response was ignored and term is not 5000. */
CuAssertIntEquals(tc, 1, raft_get_current_term(r));
}
void TestRaft_leader_recv_entry_resets_election_timeout(
......@@ -5142,7 +5169,8 @@ int main(void)
SUITE_ADD_TEST(suite, TestRaft_leader_recv_appendentries_response_do_not_increase_commit_idx_because_of_old_terms_with_majority);
SUITE_ADD_TEST(suite, TestRaft_leader_recv_appendentries_response_jumps_to_lower_next_idx);
SUITE_ADD_TEST(suite, TestRaft_leader_recv_appendentries_response_retry_only_if_leader);
SUITE_ADD_TEST(suite, TestRaft_leader_recv_appendentries_response_without_node_fails);
SUITE_ADD_TEST(suite, TestRaft_leader_recv_appendentries_response_without_node);
SUITE_ADD_TEST(suite, TestRaft_leader_recv_snapshot_response_without_node);
SUITE_ADD_TEST(suite, TestRaft_leader_recv_entry_resets_election_timeout);
SUITE_ADD_TEST(suite, TestRaft_leader_recv_entry_is_committed_returns_0_if_not_committed);
SUITE_ADD_TEST(suite, TestRaft_leader_recv_entry_is_committed_returns_neg_1_if_invalidated);
......
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