-
Notifications
You must be signed in to change notification settings - Fork 395
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
*: introduce joint quorum #382
Conversation
This PR ports joint quorum. It rename ProgressSet to ProgressTracker and keep both name and field layout just like etcd. In addition, the PR fixes a performance issue discoverred earlier that campaign prints too much logs. Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@Fullstop000 PTAL if you have time. |
src/raft.rs
Outdated
); | ||
let mut m = new_message(*id, vote_msg, None); | ||
|
||
if voter_cnt < 7 { |
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.
why 7? why just let it print all at the end even it's more than 7.
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.
To avoid allocation.
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
src/tracker.rs
Outdated
/// right away when entering the joint configuration, so that it is caught up | ||
/// as soon as possible. | ||
#[get = "pub"] | ||
learner_next: HashSet<u64>, |
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.
Prefer learners_next
to follow learners
src/raft.rs
Outdated
); | ||
let mut m = new_message(*id, vote_msg, None); | ||
|
||
if voter_cnt == 7 { |
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.
Maybe use voters.len()
instead
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.
voters is an array, its length is always 7.
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.
I think it could be better to use voter_cnt == voters.len()
to replace the magic number
src/quorum/joint.rs
Outdated
let i_idx = self.incoming.committed_index(use_group_commit, l); | ||
let o_idx = self.outgoing.committed_index(use_group_commit, l); | ||
(cmp::min(i_idx.0, o_idx.0), i_idx.1 && o_idx.1) |
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.
Maybe using tuple destructuring could be more readable?
let (i_idx, i_use_group) = self.incoming.committed_index(use_group_commit, l);
let (o_idx, o_use_group) = self.outgoing.committed_index(use_group_commit, l);
(cmp::min(i_idx, o_idx), i_use_group && o_use_group)
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
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
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
LGTM |
Thanks! @Connor1996 @Fullstop000 |
This PR ports joint quorum. It rename ProgressSet to ProgressTracker
and keep both name and field layout just like etcd.
In addition, the PR fixes a performance issue discovered earlier that
campaign prints too much logs.
joint.rs is ported from etcd/raft/quorum/joint.go and tracker is from
etcd/raft/tracker/tracker.go.