Skip to content
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

progress.committed_index may not accurate and progress.matched >= progress.committed_index may broken #426

Open
NingLin-P opened this issue Apr 7, 2021 · 4 comments

Comments

@NingLin-P
Copy link
Member

We use progress.committed_index to keep track of a peer's known commit index, but in some cases progress.committed_index may not accurate and progress.matched >= progress.committed_index may not be true, although raft-rs not rely on this assumption directly, it is pretty intuitive and can be misused by the user.

Here are some cases I found:

  1. A new leader will reset all of its prs, but only reset pr.matched = 0 not pr.committed_index
  2. After introducing async ready, the leader's pr.matched update in on_persist_entries, if a raft log committed before the leader persist it (or maybe_persist return false), the leader's pr.matched may less than its pr.committed_index
  3. The leader update the follower's pr.matched and pr.committed_index in handle_append_response, but if resp.reject is true, the leader only update the pr.committed_index
@BusyJay
Copy link
Member

BusyJay commented Apr 7, 2021

Case 1 is expected. 0 means matched is unknown. Though it can also be reset to committed_index.

Case 2 can happen. How about only updating committed_index when matched is also updated for leader? /cc @gengliqi

Why 3 is possible? If there is leadership change, pr.matched should be reset to 0. If leader is the same all the time, then follower's committed_index is always set by leader, which should always not larger than matched.

@BusyJay
Copy link
Member

BusyJay commented Apr 7, 2021

Another possible case is MsgAppendResponse from follower is dropped and committed_index is updated later by HeartbeatResponse. Maybe matched should also be updated in this case.

@gengliqi
Copy link
Member

gengliqi commented Apr 9, 2021

Case 2 can happen. How about only updating committed_index when matched is also updated for leader? /cc @gengliqi

Looks good to me.

@gengliqi
Copy link
Member

gengliqi commented Apr 9, 2021

I think maybe we don't need to maintain this invariant. Though it may make users a little bit confused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants