-
Notifications
You must be signed in to change notification settings - Fork 17
Fix pg membership inconsistency #386
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.
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
base: main
Are you sure you want to change the base?
Conversation
25ad63c to
a164d12
Compare
- Implement on_pg_clean_replace_member_task callback which actually rollback the membership change - Add a reconcile_membership API which is used to correct the existing inconsistency problem.
a164d12 to
aff362e
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #386 +/- ##
==========================================
- Coverage 63.15% 55.86% -7.29%
==========================================
Files 32 35 +3
Lines 1900 4942 +3042
Branches 204 625 +421
==========================================
+ Hits 1200 2761 +1561
- Misses 600 1888 +1288
- Partials 100 293 +193 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JacksonYao287
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| const replica_member_info& member_out, | ||
| const replica_member_info& member_in, trace_id_t tid) { | ||
| std::unique_lock lck(_pg_lock); | ||
| for (const auto& iter : _pg_map) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, suggest using get_pg_id_with_group_id instead of a loop here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have 4 similar API hook functions
- on_pg_start_replace_member
- on_pg_complete_replace_member
- reconcile_membership
- on_pg_clean_replace_member_task
all of them are trying to maintain the PG superblok in sync with actual raft membership.
So which layer maintain the source of truth ? It seems to me like the HS is the source of truth as in PG creation the PG superblock is the last one get written , pg move also sharing similar pattern.
If HS is the source of truth , can we consolidate all the API into "update_membership" ?
API hooks can still there, just trying to avoid PG superblk manage code is written several times
Rely on eBay/HomeStore#856