support handling config rollback and add periodical notfity the lastest committed lsn to upper layer #703

JacksonYao287 · 2025-04-28T02:50:42Z

No description provided.

src/include/homestore/replication/repl_dev.h

codecov-commenter · 2025-04-28T03:59:24Z

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

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 66.81%. Comparing base (1a0cef8) to head (a2ac1ab).
Report is 182 commits behind head on master.

Files with missing lines	Patch %	Lines
src/lib/replication/repl_dev/raft_repl_dev.cpp	40.00%	3 Missing ⚠️
...rc/lib/replication/repl_dev/raft_state_machine.cpp	0.00%	1 Missing ⚠️

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

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #703       +/-   ##
===========================================
+ Coverage   56.51%   66.81%   +10.29%     
===========================================
  Files         108      109        +1     
  Lines       10300    11511     +1211     
  Branches     1402     1571      +169     
===========================================
+ Hits         5821     7691     +1870     
+ Misses       3894     3066      -828     
- Partials      585      754      +169

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Besroy

LGTM

Besroy · 2025-04-28T06:11:16Z

src/include/homestore/replication/repl_dev.h

+    /// @param lsn - The log sequence number
+    ///
+    /// config change will not involve any data write, so the blkids are not passed.
+    virtual void on_config_commit(int64_t lsn){


Just curious why not set '=0' to ensure that HO implements those functions.

1 if we set it to 0, HO will have to implement this. this means before HO implements this, all the PR in homeobject will hit building failure.

2 some other upper layers(for example ,nublox) probably not care about this signal and this callback. if we make this pure virtual, all the upper layers will have to implement this callback, even if they don`t care about this.

but, yes, we can make it pure virtual, I can change it after homeobject side change is completed so that it will not block any other pr for homeobject

xiaoxichen · 2025-04-28T06:39:20Z

src/lib/replication/repl_dev/raft_repl_dev.cpp

-    // keep this variable in case it is needed later
-    (void)new_conf;
+    (void)new_conf; // use this if we need it in the future
+    m_listener->on_config_commit(lsn);


maybe passing the new_conf as (void *) ?
@yamingk what do you think?
We dont know how the config can be used on upstream without breaking the boundary for HO to understand Raft.

if we pass this as void*, when we want to use it, we have to convert it to a nuraft type, which will also break the boundary. well, IMO, we can pass it to upstream until we know we definitely need it and how to use it , changes about this will not be too expensive.

xiaoxichen · 2025-04-28T06:40:45Z

lets wait @yamingk @sanebay to review before merging this.

yuwmao · 2025-04-28T07:25:12Z

Just a reminder, HS_CTRL_REPLACE won't call on_commit as well. But it always happens with config change, so seems it's ok in GC scenario, right?

JacksonYao287 · 2025-04-28T07:42:04Z

Just a reminder, HS_CTRL_REPLACE won't call on_commit as well. But it always happens with config change,

wow, this is a very nice catch. thanks for pointing out this @yuwmao.

basically, from the perspective of nuraft, HS_CTRL_REPLACE/HS_CTRL_DESTROY are both normal(not config) log, so they should be taken into account for this case.

it's ok in GC scenario, right?
do we mean chunk gc or pg gc? I think it has nothing to do with chunk gc.

JacksonYao287 · 2025-04-28T08:52:36Z

handle op code in eBay/HomeObject#293

xiaoxichen

Overall, I question whether the effort to ensure every commit calls HO for handling GC in the context of on_commit is more beneficial than simply monitoring get_last_commit_lsn in another thread..

src/lib/replication/repl_dev/raft_repl_dev.cpp

JacksonYao287 · 2025-04-28T17:32:51Z

@xiaoxichen thanks for the great idea! I have made this change, pls take a look.

now we add notify_committed_lsn in the timer of flushing durable committed lsn, which will periodically notify the upper layer the latest committed lsn. so we change edge trigger to condition trigger on handling no_space_left. thus , we no longer need pause statemachine.

@yamingk @sanebay pls take a look

xiaoxichen · 2025-04-29T02:22:38Z

src/include/homestore/replication/repl_dev.h


+    /// @brief periodically called to notify the lastest committed lsn to the listener.
+    ///
+    /// @param lsn - The log sequence number


Concerning any heavy/blocking operations in the CB as it will block the DC_LSN flushing.

Lets start with putting clear comment here

ok ,will update this PR and add additional comments here

comments updated, ptal

xiaoxichen

lgtm.

Though I wish in GC thread model if this thread can be taken care by a global thread (across all ReplDev) in HO.

xiaoxichen · 2025-04-29T03:38:07Z

please write a decent commit message when squashing , either now or when merging.

…he lastest committed lsn to upper layer (eBay#703)

JacksonYao287 force-pushed the support-handling-commit-config branch 3 times, most recently from 297f29a to 4c45c0a Compare April 28, 2025 03:09

xiaoxichen reviewed Apr 28, 2025

View reviewed changes

src/include/homestore/replication/repl_dev.h Outdated Show resolved Hide resolved

JacksonYao287 force-pushed the support-handling-commit-config branch 2 times, most recently from 327727d to 50b29ba Compare April 28, 2025 03:25

JacksonYao287 requested review from Besroy, xiaoxichen and yuwmao April 28, 2025 04:48

Besroy previously approved these changes Apr 28, 2025

View reviewed changes

Besroy reviewed Apr 28, 2025

View reviewed changes

xiaoxichen reviewed Apr 28, 2025

View reviewed changes

JacksonYao287 mentioned this pull request Apr 28, 2025

handling config change where we also need to check no_space_left error info eBay/HomeObject#293

Merged

JacksonYao287 dismissed Besroy’s stale review via 8928910 April 28, 2025 08:11

JacksonYao287 requested review from Besroy and xiaoxichen April 28, 2025 08:55

xiaoxichen reviewed Apr 28, 2025

View reviewed changes

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

support handling rollback_config and adding notify_committed_lsn

5d27dd7

JacksonYao287 force-pushed the support-handling-commit-config branch from 2c66e28 to 5d27dd7 Compare April 28, 2025 16:43

fix ut

6ca8175

JacksonYao287 requested review from sanebay, xiaoxichen and yamingk April 28, 2025 17:32

Merge branch 'master' into support-handling-commit-config

b5f04c1

xiaoxichen previously approved these changes Apr 29, 2025

View reviewed changes

update commetns

a2ac1ab

JacksonYao287 dismissed xiaoxichen’s stale review via a2ac1ab April 29, 2025 03:34

JacksonYao287 requested a review from xiaoxichen April 29, 2025 03:34

JacksonYao287 changed the title ~~support handing config change in upper layer~~ support handling config rollback and add periodical notfity the lastest committed lsn to upper layer Apr 29, 2025

xiaoxichen approved these changes Apr 29, 2025

View reviewed changes

JacksonYao287 merged commit 31a8b42 into eBay:master Apr 29, 2025
22 checks passed

JacksonYao287 deleted the support-handling-commit-config branch April 29, 2025 04:43

hkadayam pushed a commit to hkadayam/HomeStore that referenced this pull request Aug 7, 2025

support handling config rollback and add periodical notification of t…

1a669fe

…he lastest committed lsn to upper layer (eBay#703)

support handling config rollback and add periodical notfity the lastest committed lsn to upper layer #703

support handling config rollback and add periodical notfity the lastest committed lsn to upper layer #703

Uh oh!

Conversation

JacksonYao287 commented Apr 28, 2025

Uh oh!

Uh oh!

codecov-commenter commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Besroy left a comment

Choose a reason for hiding this comment

Uh oh!

Besroy Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

JacksonYao287 Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

xiaoxichen Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

JacksonYao287 Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

xiaoxichen commented Apr 28, 2025

Uh oh!

yuwmao commented Apr 28, 2025

Uh oh!

JacksonYao287 commented Apr 28, 2025

Uh oh!

JacksonYao287 commented Apr 28, 2025

Uh oh!

xiaoxichen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JacksonYao287 commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiaoxichen Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

JacksonYao287 Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

JacksonYao287 Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

xiaoxichen left a comment

Choose a reason for hiding this comment

Uh oh!

xiaoxichen commented Apr 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Apr 28, 2025 •

edited

Loading

xiaoxichen left a comment •

edited

Loading

JacksonYao287 commented Apr 28, 2025 •

edited

Loading