commit blk during log replay in rep dev #524

raakella1 · 2024-08-24T17:49:04Z

No description provided.

codecov-commenter · 2024-08-24T18:18:19Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.45%. Comparing base (1a0cef8) to head (7652028).
Report is 51 commits behind head on master.

Files with missing lines	Patch %	Lines
src/lib/replication/repl_dev/raft_repl_dev.cpp	87.50%	1 Missing ⚠️

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

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #524       +/-   ##
===========================================
+ Coverage   56.51%   67.45%   +10.94%     
===========================================
  Files         108      109        +1     
  Lines       10300    10427      +127     
  Branches     1402     1399        -3     
===========================================
+ Hits         5821     7034     +1213     
+ Misses       3894     2717     -1177     
- Partials      585      676       +91

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

xiaoxichen · 2024-08-24T19:55:32Z

src/lib/replication/repl_dev/raft_repl_dev.cpp

    rreq->init(rkey, jentry->code, false /* is_proposer */, entry_to_hdr(jentry), entry_to_key(jentry), data_size);
    RD_LOGD("Replay log on restart, rreq=[{}]", rreq->to_string());

    if (repl_lsn > m_rd_sb->durable_commit_lsn) {
+        commit_blk(rreq);


Nit: can we dirty the allocator with these blks and still commit them in handle_commit()?. Just in the case for rollback.

Is there a way to dirty the blks without allocating them?

not yet but we can change dataservice api?

also implement roll_back to free blk

Nit: can we dirty the allocator with these blks and still commit them in handle_commit()?. Just in the case for rollback.

Just be noted commit_blk will be expecting blks in allocated state for non-recovery mode (existing handle_commit), and for recovery mode, it will try to reserve on cache also before committing to disk; Need a bit more handling to combine them.

xiaoxichen · 2024-08-24T20:14:01Z

src/lib/replication/repl_dev/raft_repl_dev.cpp

@@ -1181,10 +1186,12 @@ void RaftReplDev::on_log_found(logstore_seq_num_t lsn, log_buffer buf, void* ctx
    }

    rreq->set_lsn(repl_lsn);
+    rreq->set_lentry(lentry);


This is a bit hacky as we dont set it in normal IO path , we believe nuraft will not release them during the whole lifecycle till commit().

I would vote even for normal operations we keep lentry shared_ptr for safety and more consistent behavior.

xiaoxichen

LGTM.

We didnt catch both bugs in all our restorability UT, because both bugs occurs if we have dc_lsn < last_log_lsn , how can we have this UT to exercise this path?

yamingk · 2024-08-26T18:48:53Z

src/lib/replication/repl_dev/raft_repl_dev.cpp

@@ -1181,10 +1186,14 @@ void RaftReplDev::on_log_found(logstore_seq_num_t lsn, log_buffer buf, void* ctx
    }



Not directly related to this PR, but could impact some of the assert in underlying layer, is SM long run currently use debug build or release build?

Debug build

src/lib/replication/repl_dev/raft_repl_dev.cpp

yamingk · 2024-08-26T19:12:00Z

src/include/homestore/replication/repl_dev.h

@@ -234,6 +235,7 @@ struct repl_req_ctx : public boost::intrusive_ref_counter< repl_req_ctx, boost::
    std::variant< std::unique_ptr< uint8_t[] >, raft_buf_ptr_t > m_journal_buf; // Buf for the journal entry
    repl_journal_entry* m_journal_entry{nullptr};                               // pointer to the journal entry
    bool m_is_jentry_localize_pending{false}; // Is the journal entry needs to be localized from remote
+    nuraft::ptr< nuraft::log_entry > m_lentry;


would be nice to put a comment here that this is only needed during recovery

xiaoxichen · 2024-08-27T07:26:06Z

As discussed in DM, this PR is not right as there is no guarantee the data has been written to the allocated blks before the restart.

JacksonYao287 · 2024-08-27T08:19:18Z

        HomeRaftLogStore::end_of_append_batch(start_lsn, count);
        HISTOGRAM_OBSERVE(m_rd.metrics(), raft_end_of_append_batch_latency_us, get_elapsed_time_us(cur_time));

        cur_time = std::chrono::steady_clock::now();
        // Wait for the fetch and write to be completed successfully.
        std::move(fut).wait();

we can put std::move(fut).wait() before end_of_append_batch.

if data is written but log is not flushed, that`s fine, since the block is not committed

xiaoxichen · 2024-08-27T08:42:13Z

yes we can, it is trading regular IO latency vs space leak during recovery, which I am not sure it worth to .

Prior to that, we have to solve the rollback stuff.

JacksonYao287 · 2024-08-28T02:55:03Z

Prior to that, we have to solve the rollback stuff.

rollback happens when a request is pre_committed but not committed.
1 in normal case, we can free the blk in on_rollback.
2 in recovery case , since the blk is not commited (commit_blk will be called in on_commit), so blk leak will not happen.
in on_log_found, if the lsn of a log entry is bigger than dc_sln, we can just discard it.

any input?

xiaoxichen · 2024-08-28T18:11:32Z

discussion: https://ebay-eng.slack.com/archives/CAVQXEAKS/p1724866496708579

xiaoxichen reviewed Aug 24, 2024

View reviewed changes

raakella1 force-pushed the commit_blk branch from a57cfc9 to 6bb5ce9 Compare August 25, 2024 17:57

yamingk reviewed Aug 26, 2024

View reviewed changes

src/lib/replication/repl_dev/raft_repl_dev.cpp Show resolved Hide resolved

yamingk reviewed Aug 26, 2024

View reviewed changes

xiaoxichen previously approved these changes Aug 28, 2024

View reviewed changes

commit blk during log replay in rep dev

7652028

raakella1 dismissed xiaoxichen’s stale review via 7652028 August 28, 2024 18:20

raakella1 force-pushed the commit_blk branch from 6bb5ce9 to 7652028 Compare August 28, 2024 18:20

xiaoxichen approved these changes Aug 28, 2024

View reviewed changes

raakella1 merged commit 8d6d70b into eBay:master Aug 28, 2024
21 checks passed

raakella1 deleted the commit_blk branch August 28, 2024 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

commit blk during log replay in rep dev #524

commit blk during log replay in rep dev #524

raakella1 commented Aug 24, 2024

codecov-commenter commented Aug 24, 2024 •

edited

Loading

xiaoxichen Aug 24, 2024

raakella1 Aug 25, 2024

xiaoxichen Aug 25, 2024

JacksonYao287 Aug 26, 2024

yamingk Aug 26, 2024

xiaoxichen Aug 24, 2024

xiaoxichen left a comment

yamingk Aug 26, 2024

raakella1 Aug 27, 2024

yamingk Aug 26, 2024

xiaoxichen commented Aug 27, 2024

JacksonYao287 commented Aug 27, 2024

xiaoxichen commented Aug 27, 2024

JacksonYao287 commented Aug 28, 2024

xiaoxichen commented Aug 28, 2024

		@@ -1181,10 +1186,14 @@ void RaftReplDev::on_log_found(logstore_seq_num_t lsn, log_buffer buf, void* ctx
		}

commit blk during log replay in rep dev #524

commit blk during log replay in rep dev #524

Conversation

raakella1 commented Aug 24, 2024

codecov-commenter commented Aug 24, 2024 • edited Loading

Codecov Report

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

xiaoxichen left a 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

xiaoxichen commented Aug 27, 2024

JacksonYao287 commented Aug 27, 2024

xiaoxichen commented Aug 27, 2024

JacksonYao287 commented Aug 28, 2024

xiaoxichen commented Aug 28, 2024

codecov-commenter commented Aug 24, 2024 •

edited

Loading