Issue: 257 Replication Fetch Remote Data #290

yamingk · 2024-01-26T01:11:38Z

Changes made:

on follower, register FETCH_DATA to nuraft_mesg and add fetch remote data logic.
on leader side, handle fetch request and read data based on received remote blkid if this leader is the originator.
on follower, write the received data into drive and mark data written and trigger promise to wake up batch_append to moving forward.
introduce flip to simulate data not received from data channel (ingore data written) to trigger a fetch remote data.
added new test case to trigger fetch remote data

Testing:

unit test passed.
regression test passed.

src/lib/homestore.cpp

JacksonYao287 · 2024-01-29T11:59:42Z

src/lib/replication/repl_dev/raft_repl_dev.cpp

+        auto const& lsn = req->lsn();
+        auto const& term = req->raft_term();
+        auto const& dsn = req->dsn();
+        auto const& header = req->user_header();
+        auto const& key = req->user_key();


i do not find where are them used?

These are for the use of resync from any member purpose which will comes in next

JacksonYao287 · 2024-01-29T12:04:23Z

src/lib/replication/repl_dev/raft_repl_dev.cpp

+            MultiBlkId local_blkid;
+
+            // convert remote_blkid serialized data to local blkid
+            local_blkid.deserialize(sisl::blob{remote_blkid->Data(), remote_blkid->size()}, true /* copy */);


should we check local_blkid and remote_blkid like below to avoid some abnormal case?

HomeStore/src/lib/replication/repl_dev/raft_state_machine.cpp

Line 139 in b89f275

if (local_size > remote_size) {

They should be strictly same size, since this is a deserialize/serialize which provided by MultiBlkId itself. There is unit test case cover these apis in MultiBlkId.

JacksonYao287 · 2024-01-29T12:36:35Z

src/lib/replication/repl_dev/raft_repl_dev.cpp

+    }
+
+    // copy by value to avoid since it is on stack;
+    rpc_data->set_comp_cb([sgs_vec](boost::intrusive_ptr< sisl::GenericRpcData >&) {


if error happens when sending response, we also need to release the buffer, right?

if somehow the underlying grpc connection (which is a TCP connection) is stuck, causing comp cb can't be triggered, all connections will be stuck causing either a timeout or a reboot of this replica. Is this what you are concerning? We can discuss in "error handling" strategy as well for this as well

JacksonYao287 · 2024-01-29T12:38:50Z

src/lib/replication/repl_dev/raft_repl_dev.cpp

+                ctx->cv.notify_one();
+            });
+        } else {
+            // TODO: if we are not the originator, we need to fetch based on lsn;


do we have a lsn to multiblkId map? i think this need to be kept in metaservice or index table, since log might be purged when compaction

as we discussed, there will be a new api in listener to query index_table from homeObj to return the blkids. We don't rely on logs.

JacksonYao287 · 2024-01-29T12:39:45Z

src/lib/replication/repl_dev/raft_repl_dev.cpp

        // if in resync mode, fetch data from remote immediately;
-        check_and_fetch_remote_data(std::move(rreqs));
+        check_and_fetch_remote_data(rreqs);


is line 488 and line 490 the same?

yes, there is some problem enabling the non-recursive timer. I will have to work on them and enable them in comming PR.

JacksonYao287 · 2024-01-29T13:02:47Z

src/lib/replication/repl_dev/raft_repl_dev.cpp

+
+    if (rreqs->size()) {
+        // Some data not completed yet, let's fetch from remote;
+        fetch_data_from_remote(rreqs);


now, we do check_and_fetch_remote_data when after the log append. why can not we delay this action until we need it or using some periodical check thread to do? maybe the push_data requset is on the way

As indicated above, this the exactly what this PR is trying to do. e.g. we need the timer to wait for a configurable time of seconds before we fetch from remote.

JacksonYao287 · 2024-01-29T13:15:23Z

src/lib/replication/repl_dev/raft_repl_dev.cpp

+                    continue;
+                } else {
+                    // aquire lock here to avoid two threads are trying to do the same thing;
+                    std::unique_lock< std::mutex > lg(rreq->state_mtx);


thenValue will be executed in a single thread, right? and after each log batch append, the request are also different.
so , do we really need the lock here?

you are right about here in this context. The lock is to protect from raft log channel and data channel that could also be allocating blks (in case for some reason data arrives after this fetch remote is sent)

sanebay · 2024-01-29T23:12:19Z

src/lib/replication/repl_dev/raft_state_machine.cpp

@@ -122,6 +122,8 @@ repl_req_ptr_t RaftStateMachine::transform_journal_entry(nuraft::ptr< nuraft::lo
    sisl::blob const key = sisl::blob{header.cbytes() + header.size(), jentry->key_size};
    DEBUG_ASSERT_GT(jentry->value_size, 0, "Entry marked as large data, but value size is notified as 0");

+    LOGINFO("Received Raft server_id={}, term={}, dsn={}, journal_entry=[{}] ", jentry->server_id, lentry->get_term(),


nit: do we need this trace. as we have trace in line 116

there is some additional info in this log, I will merge them in my comming PR>

src/tests/test_raft_repl_dev.cpp

codecov-commenter · 2024-01-31T00:14:05Z

Codecov Report

Attention: 316 lines in your changes are missing coverage. Please review.

Comparison is base (64600e9) 62.60% compared to head (73b223a) 58.93%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
src/lib/replication/repl_dev/raft_repl_dev.cpp	0.00%	195 Missing ⚠️
src/lib/replication/service/raft_repl_service.cpp	0.00%	35 Missing ⚠️
...rc/lib/replication/repl_dev/raft_state_machine.cpp	0.00%	24 Missing ⚠️
src/lib/replication/service/generic_repl_svc.cpp	0.00%	14 Missing ⚠️
src/include/homestore/superblk_handler.hpp	53.57%	11 Missing and 2 partials ⚠️
src/lib/homestore.cpp	58.82%	4 Missing and 3 partials ⚠️
.../lib/replication/log_store/home_raft_log_store.cpp	0.00%	7 Missing ⚠️
src/lib/replication/repl_dev/solo_repl_dev.cpp	0.00%	7 Missing ⚠️
src/include/homestore/replication/repl_dev.h	0.00%	5 Missing ⚠️
src/lib/logstore/log_store_family.cpp	80.00%	3 Missing and 1 partial ⚠️
... and 4 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
- Coverage   62.60%   58.93%   -3.67%     
==========================================
  Files         110      110              
  Lines        8990     9254     +264     
  Branches     1156     1191      +35     
==========================================
- Hits         5628     5454     -174     
- Misses       2829     3269     +440     
+ Partials      533      531       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JacksonYao287

LGTM. thanks for the work!

yamingk and others added 8 commits January 10, 2024 15:23

resync: initial commit

4d4da2e

issue: 257 Fetch remote data

c65e89c

add wait for read complete on fetch complete

6cafa82

fix reqeust format

304b703

verfiy disable

4d46be2

Merge branch 'master' into yk_repl_fetch_remote

ce8e8e1

fix size mismatch

b8be4aa

resolve conflict

14b5cbd

yamingk linked an issue Jan 26, 2024 that may be closed by this pull request

[Resync] Fetch remote data #257

Closed

JacksonYao287 reviewed Jan 29, 2024

View reviewed changes

yamingk and others added 3 commits January 29, 2024 17:29

Merge branch 'master' into yk_repl_fetch_remote

06625ce

fix merge conflict

161cbe4

Merge branch 'master' into yk_repl_fetch_remote

b456c15

sanebay reviewed Jan 30, 2024

View reviewed changes

yamingk added 4 commits January 30, 2024 11:31

fix build

d8d1a1b

fix timer

b4252c5

disable non-recurssive tmr

9433b09

revert cmake

73b223a

JacksonYao287 previously approved these changes Feb 1, 2024

View reviewed changes

Merge branch 'master' into yk_repl_fetch_remote

65c4920

yamingk dismissed JacksonYao287’s stale review via 65c4920 February 1, 2024 01:34

JacksonYao287 approved these changes Feb 1, 2024

View reviewed changes

yamingk merged commit 42cdddb into eBay:master Feb 1, 2024
20 checks passed

yamingk deleted the yk_repl_fetch_remote branch February 1, 2024 01:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue: 257 Replication Fetch Remote Data #290

Issue: 257 Replication Fetch Remote Data #290

yamingk commented Jan 26, 2024

JacksonYao287 Jan 29, 2024

yamingk Jan 30, 2024

JacksonYao287 Jan 29, 2024

yamingk Jan 31, 2024

JacksonYao287 Jan 29, 2024

yamingk Jan 31, 2024 •

edited

Loading

JacksonYao287 Jan 29, 2024

yamingk Jan 31, 2024

JacksonYao287 Jan 29, 2024

yamingk Jan 31, 2024

JacksonYao287 Jan 29, 2024

yamingk Jan 31, 2024

JacksonYao287 Jan 29, 2024 •

edited

Loading

yamingk Jan 30, 2024

sanebay Jan 29, 2024

yamingk Feb 1, 2024

codecov-commenter commented Jan 31, 2024

JacksonYao287 left a comment

Issue: 257 Replication Fetch Remote Data #290

Issue: 257 Replication Fetch Remote Data #290

Conversation

yamingk commented Jan 26, 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

yamingk Jan 31, 2024 • edited Loading

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

JacksonYao287 Jan 29, 2024 • edited Loading

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

codecov-commenter commented Jan 31, 2024

Codecov Report

JacksonYao287 left a comment

Choose a reason for hiding this comment

yamingk Jan 31, 2024 •

edited

Loading

JacksonYao287 Jan 29, 2024 •

edited

Loading