raft: introduce group commit #359
Conversation
Every progress is assigned to a group ID. only logs replicated to at least two different groups will be committed. The algorithm won't change the quorum definition, it works more like delaying committing. Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
src/raft.rs
Outdated
| return None; | ||
| } | ||
| if self.raft_log.term(self.raft_log.committed).unwrap_or(0) != self.term { | ||
| // Reject read only request when this leader has not committed any log entry |
There was a problem hiding this comment.
Is the comment necessary? BTW it's better to extract the logic as a function.
| (vec![4, 2, 1, 3], vec![1, 0, 0, 0], 1), | ||
| (vec![4, 2, 1, 3], vec![0, 1, 0, 2], 2), | ||
| (vec![4, 2, 1, 3], vec![0, 2, 1, 0], 1), | ||
| (vec![4, 2, 1, 3], vec![1, 1, 1, 1], 1), |
There was a problem hiding this comment.
So if 4 peers are in one group, to commit a log needs 4 ACKs? I think we can ignore group ID if there is only 1 group.
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
|
I miss the requirement is two different groups. |
|
PTAL |
src/raft.rs
Outdated
| if !self.apply_to_current_term() { | ||
| return None; | ||
| } | ||
| let (index, use_group_commit) = self.prs.as_mut().unwrap().maximal_committed_index(); |
There was a problem hiding this comment.
Using self.mut_prs() is shorter
src/raft.rs
Outdated
| @@ -652,7 +721,7 @@ impl<T: Storage> Raft<T> { | |||
| /// Attempts to advance the commit index. Returns true if the commit index | |||
| /// changed (in which case the caller should call `r.bcast_append`). | |||
| pub fn maybe_commit(&mut self) -> bool { | |||
| let mci = self.prs().maximal_committed_index(); | |||
| let mci = self.prs.as_mut().unwrap().maximal_committed_index().0; | |||
src/raft.rs
Outdated
| if let Some(pr) = self.mut_prs().get_mut(*peer_id) { | ||
| pr.commit_group_id = *group_id; | ||
| } else { | ||
| return; |
There was a problem hiding this comment.
Is it valid that the peer_id in ids can not exist in this raft group?
There was a problem hiding this comment.
Yes, but here should be continue instead of returning.
| (vec![4, 2, 1, 3], vec![0, 2, 1, 0], 1), | ||
| (vec![4, 2, 1, 3], vec![1, 1, 1, 1], 2), | ||
| (vec![4, 2, 1, 3], vec![1, 1, 2, 1], 1), | ||
| (vec![4, 2, 1, 3], vec![4, 3, 2, 1], 2), |
There was a problem hiding this comment.
It makes me confused about the answer of (vec![4, 2, 1, 3], vec![1, 0, 0, 0], 1) and (vec![4, 2, 1, 3], vec![1, 1, 1, 1], 2).
Maybe we can make the logic more straightforward, such as "if the kind of group number is greater than 2, we should use group commit"
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
src/raft.rs
Outdated
| @@ -418,9 +439,6 @@ impl<T: Storage> Raft<T> { | |||
| for (_, pr) in self.mut_prs().iter_mut() { | |||
| pr.commit_group_id = 0; | |||
| } | |||
| if StateRole::Leader == self.state && self.maybe_commit() { | |||
There was a problem hiding this comment.
If using group commit, then clearing group id will make commit index become smallest one, so no need to commit; if not using group commit, clearing group id has no effect.
Every progress is assigned to a group ID. only logs replicated to at
least two different groups will be committed. The algorithm won't change
the quorum definition, it works more like delaying committing.
Close #347