-
Notifications
You must be signed in to change notification settings - Fork 18
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
Replace member commit quorum #559
Conversation
c15d4e7
to
86441e2
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #559 +/- ##
===========================================
+ Coverage 56.51% 67.20% +10.68%
===========================================
Files 108 109 +1
Lines 10300 10461 +161
Branches 1402 1407 +5
===========================================
+ Hits 5821 7030 +1209
+ Misses 3894 2749 -1145
- Partials 585 682 +97 ☔ View full report in Codecov by Sentry. |
86441e2
to
59d45ce
Compare
@sanebay lgtm , could you pls update the conan version? |
return make_async_success<>(); | ||
}); | ||
}); | ||
} | ||
|
||
void RaftReplDev::reset_quorum_size(uint32_t commit_quorum) { |
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.
do we need add some check here to disable setting those quorum sizes without intersection, since it might cause data loss or log diverging.
pls see
https://github.com/eBay/NuRaft/blob/092997ab67eb2529c5b40556e1d301c8220c315d/docs/custom_quorum_size.md?plain=1#L29
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.
this is used when we have double failure , e.g ( 2 out of 3 members are down )--- so for sure the number seems wrong.
We need other mechanism to shot on the head of other members , ensuring they are deadly dead.
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.
when we make this config change, there might be some on-going write or delete, we need to carefully handle this quorum size change.
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 need to make these PG's read only as two nodes are down. Will check with CM, in nublocks once removed they are never added back to cluster.
@@ -165,6 +171,7 @@ AsyncReplResult<> RaftReplDev::replace_member(replica_id_t member_out_uuid, repl | |||
auto err = m_state_machine->propose_to_raft(std::move(rreq)); | |||
if (err != ReplServiceError::OK) { | |||
LOGERROR("Replace member propose to raft failed {}", err); | |||
reset_quorum_size(0); |
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 also need reset_quorum_size back to 0 in 152 and 132 before we return an error
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.
Ack
4b09a0b
to
740b732
Compare
740b732
to
f3a112b
Compare
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
No description provided.