Unverified Commit 85ae16e0 authored by Ozan Tezcan's avatar Ozan Tezcan Committed by GitHub
Browse files

Handle unnecessary snapshots better (#143)

If a node receives a snapshot with snapshot_last_idx, it checks if 
it already has the snapshot and informs leader accordingly.

The problem is the receiver node may not have a snapshot but log 
entries more than snapshot_last_idx:

e.g
Leader: snapshot_last_idx : 50
Follower: snapshot_last_idx:0, last log entry index: 100

If the leader tries to send snapshot to the follower 
(with snapshot last index:50), currently, follower accepts 
the snapshot. As follower already has entries upto index 100, snapshot
is unnecessary. After delivery is complete, to load the snapshot, 
follower will call raft_begin_load_snapshot(). This function realizes
the snapshot was unnecessary and it fails. It is harmless but snapshot
delivery was unnecessary.

This PR prevents this issue earlier by checking last log index instead
 of snapshot last index.

Also, added more virtraft tests with auto-flush on. 
(This is a different pattern, it might help with detecting these issues)
parent c76eac81
......@@ -126,7 +126,19 @@ test_virtraft: $(RAFT_CFFI_TARGET)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 6 -m 3 $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 6 -m 3 --client_rate 0 $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 1 -m 3 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 1 -m 3 --client_rate 0 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 7 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 1 -m 3 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 7 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 1 -m 3 --client_rate 0 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 2 -m 3 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 2 -m 3 --client_rate 0 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 3 -m 3 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 3 -m 3 --client_rate 0 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 4 -m 3 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 4 -m 3 --client_rate 0 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 5 -m 3 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 5 -m 3 --client_rate 0 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 6 -m 3 --auto_flush $(VIRTRAFT_OPTS)
python3 tests/virtraft2.py --servers 5 -i 20000 --compaction_rate 50 --drop_rate 5 -P 10 --seed 6 -m 3 --client_rate 0 --auto_flush $(VIRTRAFT_OPTS)
.PHONY: amalgamation
amalgamation:
......
......@@ -1437,8 +1437,9 @@ int raft_recv_snapshot(raft_server_t *me,
raft_accept_leader(me, req->leader_id);
raft_reset_transfer_leader(me, 0);
/** If we already have this snapshot, inform the leader. */
if (req->snapshot_index <= me->snapshot_last_idx) {
/** If we already have the snapshot or the log entries in this snapshot,
* inform the leader. */
if (req->snapshot_index <= raft_get_current_idx(me)) {
/** Set response as if it is the last chunk to tell leader that we have
* the snapshot */
resp->last_chunk = 1;
......
......@@ -998,6 +998,51 @@ void TestRaft_set_last_chunk_on_duplicate(CuTest * tc)
CuAssertIntEquals(tc, 1, resp.last_chunk);
}
void TestRaft_set_last_chunk_if_log_is_more_advanced(CuTest * tc)
{
raft_cbs_t funcs = {
.send_snapshot = test_send_snapshot_increment,
.send_appendentries = __raft_send_appendentries,
.clear_snapshot = test_clear_snapshot,
.load_snapshot = test_load_snapshot,
.store_snapshot_chunk = test_store_snapshot_chunk,
.get_snapshot_chunk = test_get_snapshot_chunk
};
struct test_data data = {0};
void *r = raft_new();
raft_set_current_term(r, 1);
raft_set_callbacks(r, &funcs, &data);
raft_add_node(r, NULL, 1, 1);
__RAFT_APPEND_ENTRY(r, 1, 1, "entry");
__RAFT_APPEND_ENTRY(r, 2, 1, "entry");
__RAFT_APPEND_ENTRY(r, 4, 1, "entry");
__RAFT_APPEND_ENTRY(r, 5, 1, "entry");
__RAFT_APPEND_ENTRY(r, 6, 1, "entry");
raft_snapshot_req_t msg = {
.term = 1,
.leader_id = 2,
.snapshot_index = 5,
.snapshot_term = 1,
.msg_id = 1,
.chunk.data = "tmp",
.chunk.offset = 0,
.chunk.len = 50,
.chunk.last_chunk = 1,
};
raft_snapshot_resp_t resp = {0};
raft_recv_snapshot(r, NULL, &msg, &resp);
CuAssertIntEquals(tc, 0, data.store_chunk);
CuAssertIntEquals(tc, 0, data.load);
CuAssertIntEquals(tc, 1, resp.success);
CuAssertIntEquals(tc, 1, resp.last_chunk);
}
void TestRaft_restore_after_restart(CuTest * tc)
{
raft_server_t *r = raft_new();
......@@ -1042,6 +1087,7 @@ int main(void)
SUITE_ADD_TEST(suite, TestRaft_clear_snapshot_on_leader_change);
SUITE_ADD_TEST(suite, TestRaft_reject_wrong_offset);
SUITE_ADD_TEST(suite, TestRaft_set_last_chunk_on_duplicate);
SUITE_ADD_TEST(suite, TestRaft_set_last_chunk_if_log_is_more_advanced);
SUITE_ADD_TEST(suite, TestRaft_restore_after_restart);
CuSuiteRun(suite);
......
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