Skip to content

implement gc thread model #285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
May 16, 2025
Merged

implement gc thread model #285

merged 7 commits into from
May 16, 2025

Conversation

JacksonYao287
Copy link
Collaborator

@JacksonYao287 JacksonYao287 commented Apr 17, 2025 •
edited
Loading

this is only framwork, will add UT after we have data copy logic

@JacksonYao287 JacksonYao287 force-pushed the gc-thread-model branch 4 times, most recently from dbd62d7 to 317da1f Compare April 18, 2025 06:40
@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2025 •
edited
Loading

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

Codecov Report

Attention: Patch coverage is 36.75418% with 265 lines in your changes missing coverage. Please review.

Project coverage is 58.65%. Comparing base (1746bcc) to head (01901d2).
Report is 72 commits behind head on main.

Files with missing lines Patch % Lines
src/lib/homestore_backend/gc_manager.cpp 22.85% 185 Missing and 4 partials ⚠️
src/lib/homestore_backend/index_kv.hpp 16.66% 15 Missing ⚠️
src/lib/homestore_backend/hs_blob_manager.cpp 50.00% 12 Missing ⚠️
src/lib/blob_route.hpp 0.00% 11 Missing ⚠️
src/lib/homestore_backend/index_kv.cpp 67.74% 9 Missing and 1 partial ⚠️
src/lib/homestore_backend/hs_shard_manager.cpp 43.75% 9 Missing ⚠️
src/lib/homestore_backend/heap_chunk_selector.cpp 69.23% 6 Missing and 2 partials ⚠️
src/lib/homestore_backend/hs_homeobject.cpp 80.00% 5 Missing and 3 partials ⚠️
src/lib/homestore_backend/heap_chunk_selector.h 0.00% 1 Missing ⚠️
src/lib/homestore_backend/hs_homeobject.hpp 66.66% 1 Missing ⚠️
... and 1 more

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

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #285      +/-   ##
==========================================
- Coverage   63.15%   58.65%   -4.51%     
==========================================
  Files          32       34       +2     
  Lines        1900     3391    +1491     
  Branches      204      400     +196     
==========================================
+ Hits         1200     1989     +789     
- Misses        600     1193     +593     
- Partials      100      209     +109

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

@JacksonYao287 JacksonYao287 force-pushed the gc-thread-model branch 2 times, most recently from 346e250 to ccaea57 Compare April 20, 2025 03:15
@JacksonYao287 JacksonYao287 force-pushed the gc-thread-model branch 2 times, most recently from bc099bf to 9ffc30d Compare April 30, 2025 09:32
xiaoxichen
xiaoxichen reviewed May 7, 2025
View reviewed changes
Copy link
Collaborator

@xiaoxichen xiaoxichen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The data copy logic is not fully done so just comment a bit based on what we have.
Data copy logic provides good details which is helpful for understanding the thread model however when merging lets try to separate them

JacksonYao287
JacksonYao287 commented May 8, 2025
View reviewed changes
Copy link
Collaborator Author

@JacksonYao287 JacksonYao287 left a comment

Choose a reason for hiding this comment

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

thanks @xiaoxichen for the comments, I will rebase and update this PR

src/lib/homestore_backend/gc_manager.cpp Outdated
task.complete(false);
return;
}
auto selected_chunk = m_chunk_selector->select_specific_chunk(move_from_vchunk->m_pg_id.value(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is good point. I thought select_specific_chunk will take a chunk out of the chunk heap and not available for creating a shard , seems the logic here has been changed.

I will thing about this , thanks.

@JacksonYao287 JacksonYao287 force-pushed the gc-thread-model branch 3 times, most recently from aa835fd to acfa761 Compare May 12, 2025 11:08
xiaoxichen
xiaoxichen reviewed May 14, 2025
View reviewed changes
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.

lgtm in general

@JacksonYao287
Copy link
Collaborator Author

JacksonYao287 commented May 15, 2025 •
edited
Loading

@xiaoxichen I have removed the fun you mentioned , ptal

@JacksonYao287 JacksonYao287 requested a review from xiaoxichen May 15, 2025 10:10
@JacksonYao287 JacksonYao287 merged commit 14149ae into eBay:main May 16, 2025
25 checks passed
@JacksonYao287 JacksonYao287 deleted the gc-thread-model branch May 16, 2025 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoxichen xiaoxichen xiaoxichen approved these changes

@Hooper9973 Hooper9973 Awaiting requested review from Hooper9973

@yuwmao yuwmao Awaiting requested review from yuwmao

@yamingk yamingk Awaiting requested review from yamingk

@sanebay sanebay Awaiting requested review from sanebay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JacksonYao287 @codecov-commenter @xiaoxichen