Add stream functional test and fix bugs #532

ZZhongge · 2024-09-10T10:32:42Z

add four test for stream function
fix throttling, lock, runtime change, and enable stream

1. add four test for stream function 2. fix throttling, lock, runtime change, and enable stream

ZZhongge · 2024-09-10T10:34:07Z

src/handle_append_entries.cxx

        int32 max_gap_in_stream = ctx_->get_params()->max_log_gap_in_stream_;
        ulong acceptable_precommit_idx = resp.get_next_idx() +
                                         max_gap_in_stream;
        ulong last_streamed_log_idx = p->get_last_streamed_log_idx();
+        p_tr("max gap: %d, acceptable_precommit_idx: %ld, last_streamed_log_idx: %ld, "
+             "last_sent: %ld, next_idx: %ld", max_gap_in_stream, 
+             acceptable_precommit_idx, last_streamed_log_idx, p->get_last_sent_idx(), 


We need to check whether it should enable stream first. Because calling commit will change p->get_last_sent_idx()

ZZhongge · 2024-09-10T10:36:30Z

tests/unit/stream_functional_test.cxx

+    CHK_Z( append_log_in_stream_without_delivery(s1, s2_addr, 1, 1) );
+
+    // Activate stream mode
+    // It need to re-install the snapshot because of config change, 18 reqs


After S2 re-enter into the cluster, it will trigger a duplicate snapshot install, is that expected?

Because in S2, at first it received snapshot at 10, and had log from 11 to 14.

2024-09-18T17:07:47.820_650-07:00 [159298] [INFO] snapshot idx 10 term 1 is successfully applied, log start 11 last idx 14 [handle_snapshot_sync.cxx:593, handle_snapshot_sync_req()]

after that, log 15 was created for configuration in S1

2024-09-18T17:07:47.820_938-07:00 [159303] [INFO] new configuration: log idx 15, prev log idx 14peer 1, DC ID 1, S1, voting member, 50 peer 2, DC ID 1, S2, voting member, 50 my id: 1, leader: 1, term: 1 [handle_commit.cxx:929, reconfigure()]

since snapshot distance is 5, and number of reserved log is 0, it immediately did log compaction, up to 15

2024-09-18T17:07:47.820_962-07:00 [159303] [INFO] snapshot idx 15 log_term 1 created, compact the log store if needed [handle_commit.cxx:662, on_snapshot_completed()] 2024-09-18T17:07:47.820_964-07:00 [159303] [INFO] log_store_ compact upto 15 [handle_commit.cxx:673, on_snapshot_completed()]

which means, there is no way for S2 to catch-up, hence snapshot was sent again.

2024-09-18T17:07:47.821_256-07:00 [159298] [INFO] trying to sync snapshot with last index 15 to peer 2, its last log idx 14 [handle_snapshot_sync.cxx:144, create_sync_snapshot_req()]

So if you want to avoid sending snapshot twice, set reserved_log_items_ greater than 0 at the beginning of the test. Maybe 5 is appropriate.

greensky00 · 2024-09-18T00:11:57Z

src/handle_append_entries.cxx

@@ -284,10 +284,17 @@ bool raft_server::request_append_entries(ptr<peer> p) {
        }
    } else {
        ulong last_streamed_log_idx = p->get_last_streamed_log_idx();
+        int32 max_gap_in_stream = ctx_->get_params()->max_log_gap_in_stream_;


Instead of ctx_->get_params(), use params which is gathered above, to avoid calling get_params() twice.

greensky00 · 2024-09-18T00:22:13Z

src/handle_append_entries.cxx

@@ -284,10 +284,17 @@ bool raft_server::request_append_entries(ptr<peer> p) {
        }
    } else {
        ulong last_streamed_log_idx = p->get_last_streamed_log_idx();
+        int32 max_gap_in_stream = ctx_->get_params()->max_log_gap_in_stream_;
+        if (last_streamed_log_idx > 0 && max_gap_in_stream == 0) {
+            p_in("disable stream mode at runtime");


Put more useful info: peer id, last_streamed_log_idx.

greensky00 · 2024-09-18T00:24:10Z

src/handle_append_entries.cxx

+                    if (max_gap_in_stream + p->get_next_log_idx() 
+                        <= (last_streamed_log_idx + 1)) {


if (max_gap_in_stream + p->get_next_log_idx() <= (last_streamed_log_idx + 1)) {

greensky00 · 2024-09-18T00:30:55Z

src/handle_append_entries.cxx

+        p_tr("max gap: %d, acceptable_precommit_idx: %ld, last_streamed_log_idx: %ld, "
+             "last_sent: %ld, next_idx: %ld", max_gap_in_stream, 


For ulong (== uint64_t), we should use PRIu64 to be multi platform compatible. Also need peer ID in this log.

greensky00 · 2024-09-18T00:31:16Z

src/handle_append_entries.cxx

        if (max_gap_in_stream > 0 &&
            last_streamed_log_idx == 0 && 
            resp.get_next_idx() > 0 &&
            p->get_last_sent_idx() < resp.get_next_idx() && 
            precommit_index_ < acceptable_precommit_idx) {
+            p_in("start stream mode at idx: %ld", resp.get_next_idx() - 1);


Ditto: PRIu64

And also put peer ID here too.

greensky00 · 2024-09-18T00:35:53Z

tests/unit/stream_functional_test.cxx

@@ -0,0 +1,573 @@
+/************************************************************************
+Copyright 2017-2019 eBay Inc.


It should be 2017-present

greensky00 · 2024-09-18T00:36:09Z

tests/unit/stream_functional_test.cxx

@@ -0,0 +1,573 @@
+/************************************************************************
+Copyright 2017-2019 eBay Inc.
+Author/Developer(s): Jung-Sang Ahn


You can either remove this line, or put your name.

greensky00 · 2024-09-19T00:03:54Z

tests/unit/stream_functional_test.cxx

+                   bool enable_ssl,
+                   bool use_global_asio = false,
+                   bool use_bg_snapshot_io = true,
+                   const raft_server::init_options& opt = raft_server::init_options()) {


greensky00 · 2024-09-19T00:06:18Z

tests/unit/stream_functional_test.cxx

+
+    // Snapshot transmission in stream mode
+    ts.doTest( "snapshot transmission in stream mode",
+            snapshot_transmission_in_stream_mode );


greensky00 · 2024-09-19T00:44:44Z

tests/unit/stream_functional_test.cxx

+    CHK_Z( append_log_in_stream_without_delivery(s1, s2_addr, 1, 1) );
+
+    // Activate stream mode
+    // It need to re-install the snapshot because of config change, 18 reqs


Because in S2, at first it received snapshot at 10, and had log from 11 to 14.

2024-09-18T17:07:47.820_650-07:00 [159298] [INFO] snapshot idx 10 term 1 is successfully applied, log start 11 last idx 14 [handle_snapshot_sync.cxx:593, handle_snapshot_sync_req()]

after that, log 15 was created for configuration in S1

2024-09-18T17:07:47.820_938-07:00 [159303] [INFO] new configuration: log idx 15, prev log idx 14peer 1, DC ID 1, S1, voting member, 50 peer 2, DC ID 1, S2, voting member, 50 my id: 1, leader: 1, term: 1 [handle_commit.cxx:929, reconfigure()]

since snapshot distance is 5, and number of reserved log is 0, it immediately did log compaction, up to 15

2024-09-18T17:07:47.820_962-07:00 [159303] [INFO] snapshot idx 15 log_term 1 created, compact the log store if needed [handle_commit.cxx:662, on_snapshot_completed()] 2024-09-18T17:07:47.820_964-07:00 [159303] [INFO] log_store_ compact upto 15 [handle_commit.cxx:673, on_snapshot_completed()]

which means, there is no way for S2 to catch-up, hence snapshot was sent again.

2024-09-18T17:07:47.821_256-07:00 [159298] [INFO] trying to sync snapshot with last index 15 to peer 2, its last log idx 14 [handle_snapshot_sync.cxx:144, create_sync_snapshot_req()]

So if you want to avoid sending snapshot twice, set reserved_log_items_ greater than 0 at the beginning of the test. Maybe 5 is appropriate.

ZZhongge · 2024-09-24T16:33:57Z

src/raft_server.cxx

@@ -1047,6 +1047,7 @@ void raft_server::become_leader() {
            // reconnect_client(*pp);

            pp->set_next_log_idx(log_store_->next_slot());
+            pp->reset_stream();


Shall we need to also disable the streaming when it become follower?

Yes, makes sense.

greensky00 · 2024-09-24T18:18:11Z

Please make a fix on peer (regarding the edge case that connection becomes new one) in a separate PR.

Add stream functional test and fix bugs

8d4505b

1. add four test for stream function 2. fix throttling, lock, runtime change, and enable stream

ZZhongge commented Sep 10, 2024

View reviewed changes

greensky00 reviewed Sep 19, 2024

View reviewed changes

lihzeng added 2 commits September 24, 2024 06:25

handle comment

060c6df

Need disable stream when become the leader

2812cf5

ZZhongge commented Sep 24, 2024

View reviewed changes

greensky00 approved these changes Sep 24, 2024

View reviewed changes

greensky00 merged commit 3835141 into eBay:streaming Sep 24, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stream functional test and fix bugs #532

Add stream functional test and fix bugs #532

ZZhongge commented Sep 10, 2024

ZZhongge Sep 10, 2024

ZZhongge Sep 10, 2024

greensky00 Sep 19, 2024

greensky00 Sep 18, 2024

greensky00 Sep 18, 2024

greensky00 Sep 18, 2024

greensky00 Sep 18, 2024

greensky00 Sep 18, 2024

greensky00 Sep 18, 2024

greensky00 Sep 18, 2024

greensky00 Sep 18, 2024

greensky00 Sep 19, 2024

greensky00 Sep 19, 2024

greensky00 Sep 19, 2024

ZZhongge Sep 24, 2024

greensky00 Sep 24, 2024

greensky00 commented Sep 24, 2024

		if (max_gap_in_stream + p->get_next_log_idx()
		<= (last_streamed_log_idx + 1)) {

		p_tr("max gap: %d, acceptable_precommit_idx: %ld, last_streamed_log_idx: %ld, "
		"last_sent: %ld, next_idx: %ld", max_gap_in_stream,

		@@ -0,0 +1,573 @@
		/************************************************************************
		Copyright 2017-2019 eBay Inc.

Add stream functional test and fix bugs #532

Add stream functional test and fix bugs #532

Conversation

ZZhongge commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greensky00 commented Sep 24, 2024