cockroach icon indicating copy to clipboard operation
cockroach copied to clipboard

raft: remove commit index advancement dependence on heartbeats

Open iskettaneh opened this issue 1 year ago • 2 comments

This commit removes the commit index advancement's dependence on heartbeats. It does so by making sure that the leader sends an empty MsgApp request if it knows that the follower's commit index is stale.

Backward compatability arguments:

  1. Followers can accept commit index advancement using both paths: Heartbeats & MsgApps.

  2. If the leader's point of view of the cluster version is old, it uses the old path of advancing the followrs' commit index using heartbeats.

  3. If the leader's point of view of the cluster version is new, it means that all nodes in the cluster (including all followers) have the latest binary for sure. At that moment, the leader uses the new logic of advancing the commit index using MsgApp rather than heartbeats.

Fixes: https://github.com/cockroachdb/cockroach/issues/125266

Epic: None

Release note: None

iskettaneh avatar Aug 22 '24 17:08 iskettaneh

This change is Reviewable

cockroach-teamcity avatar Aug 22 '24 17:08 cockroach-teamcity

pkg/raft/tracker/progress.go line 350 at r2 (raw file): I didn't combine them into one condition because the two condition are slightly different:

  1. The SentCommit as Pavel mentioned is not paced. Meaning that we can send two back-to-back messages if we know that the SentCommit is not to it's correct value. We can keep sending empty MsgApp until we advance the SentCommit all the way.
  2. The new condition is paced. we don't want to keep sending empty MsgApp if we haven't received a MsgAppResp from a follower. That message might take some time until the leader receives it, and therefore it needs to be paced.

Regarding

But if advanceCommitViaMsgAppOnly then additionally bump minCommit "optimistically" when sending MsgApp.

I don't think we can advance the MinCommit in that case because the MsgApp might get dropped, and then if we advance the MinCommit, when will the leader send another MsgApp?

iskettaneh avatar Aug 28 '24 17:08 iskettaneh

bors r+

iskettaneh avatar Aug 30 '24 17:08 iskettaneh