search

eBay / HomeStore

Storage Engine for block and key/value stores.
Apache License 2.0
23 stars 21 forks source link

push data asynchronously at leader #590

Closed JacksonYao287 closed 2 weeks ago

JacksonYao287 commented 2 weeks ago

now we always start appending log to log store at leader until all the futures of pushing data to followers are completed , which will involve unnecessary latency

this PR aims to make pushing data asynchronously, so that logs can be set to follower ASAP. it does not matter whether push_data is successful, since follower will try to fetch data from the leader if push_data fails

codecov-commenter commented 2 weeks ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

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

Project coverage is 67.17%. Comparing base (1a0cef8) to head (dd3156d). Report is 91 commits behind head on master.

Files with missing lines Patch % Lines
src/lib/replication/repl_dev/raft_repl_dev.cpp 66.66% 1 Missing :warning:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #590 +/- ## =========================================== + Coverage 56.51% 67.17% +10.65% =========================================== Files 108 109 +1 Lines 10300 10729 +429 Branches 1402 1466 +64 =========================================== + Hits 5821 7207 +1386 + Misses 3894 2824 -1070 - Partials 585 698 +113 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JacksonYao287 commented 2 weeks ago

it seems right but I would like to hold this till we solve and bake a stable version for PG move.

sure, will also loop other members in

xiaoxichen commented 2 weeks ago

one question: the push_data_to_all_followers returns a future, we dont wait for the future at all, what is the difference compared to run_on_forget

JacksonYao287 commented 2 weeks ago

we dont wait for the future at all

we use folly::collectAllUnsafe to wait for the completion of all the future, no?

xiaoxichen commented 2 weeks ago

collectAllUnsafe

https://github.com/facebook/folly/blob/main/folly/futures/Future.h#L2360-L2385 it returns a future which satisfied when all to_be_collected features satisfied.

JacksonYao287 commented 2 weeks ago

collectAllUnsafe

I see , you are right. I misunderstood the meaning of collectAllUnsafe. now I think , as you said , there is no difference compared to run_on_forget and the only improvement in this PR is that allocating_buffer and releasing buffer will not block the current thread, which seems not worth a change. let me close this pr