etcd-io / raft

Raft library for maintaining a replicated state machine
Apache License 2.0
630 stars 160 forks source link

tracker: track in-flight commit index #171

Closed pav-kv closed 6 months ago

pav-kv commented 6 months ago

This commit adds a Progress.sentCommit field tracking the highest commit index which the leader sent to the follower. It is used to distinguish cases when a commit index update needs or doesn't need to be sent to a follower.

Touches #131

pav-kv commented 6 months ago

@ahrtr PTAL. Forked this from #134. This reduces the amount of unnecessary empty MsgApp messages, see the effect on the tests in testdata.

pav-kv commented 6 months ago

@serathius Can I ask you to review this too? CRDB reviewers are OOO this week.

pav-kv commented 6 months ago

@bdarnell This addresses your comment to a good degree, without changing the protocol.

pav-kv commented 6 months ago

@serathius Could you take a look please?

pav-kv commented 6 months ago

@ahrtr Are you expecting a second review? Can this be merged? Alternatively, could you start reviewing #134 that's built on top of this PR (just ignore the first two commits which are this PR)?

ahrtr commented 6 months ago

@ahrtr Are you expecting a second review?

Yes, it's definitely better to have a second review for any behaviour or logic change. We need to minimize any risk as much as possible.

For any mechanical change, such as just renaming or simple code refactor, it's OK to merge with only one approval. Such as https://github.com/etcd-io/raft/pull/172

could you start reviewing #134

It's in my to-do list. Will take a look later.

pav-kv commented 6 months ago

@serathius ping

pav-kv commented 6 months ago

@ptabor @spzala Could you review/approve please?

pav-kv commented 6 months ago

@ahrtr All actionable comments in this PR are resolved, and the open discussion motivates a second PR #132 after this one. This PR is ready for merge.

ahrtr commented 6 months ago

Please also rebase this PR to resolve the workflow failure.

pav-kv commented 6 months ago

@ahrtr done