Skip to content

Conversation

@sanebay
Copy link
Contributor

@sanebay sanebay commented Apr 28, 2025

Add support for async write data, journal, alloc blks for solo repl dev. Raft repl dev doesnt support these operations. This is needed for nublocks where it need to write free blkids also to the journal. Free blocks are obtained after writing the new blkids to index. Add apis for allocation and write for vector of blkids . Raft repldev currently uses only a single blkid. Test solo repl dev changes to support vector of blkids.
TODO
check if we can merge the alloc_blks(multiblkid) and alloc_blks(vector).
Also merge the local_blkid() and local_blkids() in raft repl dev.
issue created to track this.

@sanebay sanebay force-pushed the solo_repl_multiple_blkids branch 5 times, most recently from e65f40a to 6ab6ab1 Compare April 29, 2025 22:26
@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2025

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

Codecov Report

Attention: Patch coverage is 78.51240% with 26 lines in your changes missing coverage. Please review.

Project coverage is 66.82%. Comparing base (1a0cef8) to head (06fbe7d).
Report is 187 commits behind head on master.

Files with missing lines Patch % Lines
src/lib/replication/repl_dev/solo_repl_dev.cpp 75.40% 10 Missing and 5 partials ⚠️
src/lib/replication/repl_dev/raft_repl_dev.h 0.00% 6 Missing ⚠️
src/lib/blkdata_svc/blkdata_service.cpp 87.50% 2 Missing ⚠️
src/include/homestore/replication/repl_dev.h 85.71% 0 Missing and 1 partial ⚠️
src/lib/replication/repl_dev/common.cpp 95.83% 1 Missing ⚠️
src/lib/replication/repl_dev/raft_repl_dev.cpp 85.71% 1 Missing ⚠️

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

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #706       +/-   ##
===========================================
+ Coverage   56.51%   66.82%   +10.31%     
===========================================
  Files         108      109        +1     
  Lines       10300    11947     +1647     
  Branches     1402     1665      +263     
===========================================
+ Hits         5821     7984     +2163     
+ Misses       3894     3127      -767     
- Partials      585      836      +251     

☔ 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.

Copy link
Collaborator

@xiaoxichen xiaoxichen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we get a feeling that solo dev is no longer "like" the repl dev?

Previously the solo_repl_dev was written as a phototype for integration IIUC. Now it becomes first class citizen, then the question comes to as it diverse from the entry (async_write vs async_alloc_write, write_journal etc), implementation, logic, also the consumer (user of repl dev).

What is the benefit of putting them under same base class? @yamingk

@sanebay
Copy link
Contributor Author

sanebay commented Apr 30, 2025

Previously the solo_repl_dev was written as a phototype for integration IIUC. Now it becomes first class citizen, then the question comes to as it diverse from the entry (async_write vs async_alloc_write, write_journal etc), implementation, logic, also the consumer (user of repl dev).

solo repl dev was not prototype. Hari planned to use it for nublocks. We didnt go to details at that time and assumed async_alloc_write would be enough. But with detail design, we found that we will need this additional api's

@xiaoxichen
Copy link
Collaborator

Previously the solo_repl_dev was written as a phototype for integration IIUC. Now it becomes first class citizen, then the question comes to as it diverse from the entry (async_write vs async_alloc_write, write_journal etc), implementation, logic, also the consumer (user of repl dev).

solo repl dev was not prototype. Hari planned to use it for nublocks. We didnt go to details at that time and assumed async_alloc_write would be enough. But with detail design, we found that we will need this additional api's

OK, as far as you are still seeing its beneficial to stay under the repl_dev umbrella instead of feeling burden to refactor/keep consistency with raft_repl_dev, I am good with either.

@yamingk
Copy link
Contributor

yamingk commented Apr 30, 2025

Can we document the one todo here in PR description that we plan to even simplify the interface further by merging local_blkid and local_blkIds into one, have the issue number as well.

Also can we also summerize the discussion we had as well for the possibility to even mrege the two alloc_blks in repl_dev.h, e.g.

  1. alloc_blks with MultiBlkId& out_bid
  2. alloc_blks with std::vector<MultiBlkId>& out_bids

We didn't do that in this PR as we are not sure wether NuObject use case would prefer to use a standalone interface or have a combined one for better readability (e.g. why append blk allocator would need a vector of bids), so we didn't make this merge at the moment. cc @xiaoxichen

@sanebay
Copy link
Contributor Author

sanebay commented Apr 30, 2025

Can we document the one todo here in PR description that we plan to even simplify the interface further by merging local_blkid and local_blkIds into one, have the issue number as well.

Agree to the comment above. I have added a todo in commit description about the issue created. We should simplify in later PR if needed.

@yamingk
Copy link
Contributor

yamingk commented Apr 30, 2025

LGTM, after the build failure is fixed in CI

@sanebay sanebay force-pushed the solo_repl_multiple_blkids branch from 6ab6ab1 to 4e4c4b5 Compare April 30, 2025 22:43
Add support for async write data, journal, alloc blks for solo
repl dev. Raft repl dev doesnt support these operations.
This is needed for nublocks where it need to write
free blkids also to the journal. Free blocks are obtained
after writing the new blkids to index. Add apis for allocation
and write for vector of blkids . Raft repldev currently uses only
a single blkid. Test solo repl dev changes to support vector of blkids.
@sanebay sanebay force-pushed the solo_repl_multiple_blkids branch from 4e4c4b5 to 06fbe7d Compare May 1, 2025 00:37
@sanebay sanebay merged commit 679e8fb into eBay:master May 1, 2025
21 checks passed
@sanebay sanebay deleted the solo_repl_multiple_blkids branch May 1, 2025 17:09
hkadayam pushed a commit to hkadayam/HomeStore that referenced this pull request Aug 7, 2025
Add support for async write data, journal, alloc blks for solo
repl dev. Raft repl dev doesnt support these operations.
This is needed for nublocks where it need to write
free blkids also to the journal. Free blocks are obtained
after writing the new blkids to index. Add apis for allocation
and write for vector of blkids . Raft repldev currently uses only
a single blkid. Test solo repl dev changes to support vector of blkids.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants