raft: consolidate all append message sending #134

pav-kv · 2024-01-25T13:41:17Z

This PR consolidates all decision-making about sending append messages into a single maybeSendAppend method. Previously, the behaviour depended on the sendIfEmpty flag which was set/unset depending on the circumstances in which the method is called. This is unnecessary because the Progress struct contains enough information about the leader->follower flow state, so maybeSendAppend can be made stand-alone.

In a follow-up PR, the consolidated maybeSendAppend method will be used to implement a more flexible message flow control.

Part of #130

tracker/progress.go

raft.go

nvanbenschoten · 2024-02-13T21:14:19Z

raft.go

+		Type:    pb.MsgApp,
+		Index:   pr.Next - 1,
+		LogTerm: prevTerm,
+		Commit:  r.raftLog.committed,


I'm noticing the odd asymmetry between MsgApp sending the latest Commit index and the follower computing min(committed, lastnewi), and MsgHeartbeat carefully sending min(pr.Match, r.raftLog.committed) to avoid a follower-side panic. Do you have thoughts about that?

Yes. The commit index in MsgApp is "optimistic", and can be larger than pr.Match. As a result, a follower can simultaneously append and commit entries. Technically, Commit index can even exceed the last index of the entries in the batch. The way I think about MsgApp is that it's effectively 2 independent messages:

A logSlice of the leader log to append to the follower log.

A (term, commit) is the point in the leader@term log that is committed (see The notion of Term is overloaded #144 explaining the "leader log" coordinate system).

As such, even if the append (1) is redundant/no-op (because the follower already has these entries from elsewhere), the commit index (2) can still communicate valuable information - if the follower log is at the same term, it can advance the commit index safely. We don't take advantage of this currently (the commit index is advanced only within the bounds of the appended entries (1)), but we should.

The commit index in MsgHeartbeat is "pessimistic" and cut at pr.Match because currently the follower has a panic if the index is not in the bounds of the follower log. This pessimistic cut and panic are unnecessary though, see #138 and #139 aiming to relax it.

I would like to clean-up the semantics of the Commit indices in both messages, to both be treated as (2) above - a (term, index) that the leader sends without relying on the knowledge about the follower log. The follower log has enough knowledge to apply (or ignore) it safely.

tracker/progress.go

raft.go

pav-kv · 2024-02-26T13:27:16Z

@ahrtr PTAL. Also asking @bdarnell to review (@nvanbenschoten and @erikgrinaker can't look at at it this week).

bdarnell · 2024-02-26T15:42:16Z

Looks reasonable to me, although I'm very far removed from this code and I'm not sure I'm qualified to give it a thorough review. In particular I can see from the tests that there are behavior changes here, but I can't say for sure whether they're all safe.

One thing this highlights for me is how the piggybacking of (unacknowledged) commit indexes onto MsgApp is a source of trouble. I've been tempted in the past to introduce something like a new pair of messages MsgCommitIndex/MsgCommitIndexResp so the leader always knows the commit index that the follower has acknowledged, and we can reduce some of the special cases around MsgApp. I don't know if the time is right for that but it's something to consider as we're improving flow control here.

ahrtr · 2024-02-26T15:48:37Z

Will take a closer look this week.

A generic comment, in order to make the review easier, suggest to breakdown this PR. Such as,

get code refactor, method/function renaming, comment updating etc. in one PR, so that they can be reviewed and merged asap.
If you change any logic or adding any new field, then it needs super careful review, please try to make such PR as small as possible (e.g. < 100 lines of code change).

pav-kv · 2024-02-26T15:49:58Z

@bdarnell We have an issue for the commit index acks you described: #131. And a draft PR for it: #132. It's not necessary for the flow control change here though.

tracker/progress.go

ahrtr · 2024-02-26T19:12:28Z

tracker/progress.go

 	}
+	pr.PauseMsgAppProbes(true)


It seems that it changes the original logic. In previous implementation, if pr.State == StateReplicate && !pr.Inflights.Full() is true, then it will not pause the flow. Your new implementation will pause the flow in this case. Is it an intentional change?

This PR modifies the meaning of the MsgAppFlowPaused bool, but the visible behaviour of MsgApp sends is not changed.

With this PR, the pr.Inflights.Full() check just moved to a different place: see the CanSendEntries and ShouldSendAppend funcs.

It's not super intentional. I tried a few different ways, and didn't find a way that doesn't modify the meaning of this bool at all. This bool is not used by user code though, and we should unexport it IMO.

ahrtr · 2024-02-26T19:36:50Z

tracker/progress.go

+		return pr.CanBumpCommit(commit) ||
+			pr.Match < last && (!pr.MsgAppProbesPaused || pr.CanSendEntries(last))


The condition is a little over complicated. Is it possible that both pr.MsgAppProbesPaused and pr.CanSendEntries(last) are true? ShouldSendMsgApp may still return true in such case?

Yes, it's possible, we set MsgAppProbesPaused after every message send for instance. MsgAppProbesPaused == true means: if we are in the probing/throttled mode, don't send a message. But if we are not in this mode (CanSendEntries is true) then MsgAppProbesPaused is effectively ignored.

We could be keeping MsgAppProbesPaused in sync with CanSendEntries, but it would require some additional invariants.

raft.go

Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

This commit consolidates all decision-making about sending append messages into a single maybeSendAppend method. Previously, the behaviour depended on the sendIfEmpty flag which was set/unset depending on the circumstances in which the method is called. This is unnecessary because the Progress struct contains enough information about the leader->follower flow state, so maybeSendAppend can be made stand-alone. Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

124006: raft: consolidate all append message sending r=nvanbenschoten a=pav-kv This PR consolidates all decision-making about sending append messages into a single `maybeSendAppend` method. Previously, the behaviour depended on the `sendIfEmpty` flag which was set/unset depending on the context in which the method is called. This is unnecessary because the `Progress` struct contains enough information about the leader->follower flow state, so `maybeSendAppend` can be made stand-alone. In follow-up PRs, the consolidated `maybeSendAppend` method will be used to implement a more flexible message flow control. Ported from etcd-io/raft#134 Epic: CRDB-37515 Release note: none Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

pav-kv · 2024-05-29T13:20:12Z

Closing this PR, since it has been superseded by cockroachdb/cockroach#124006 in CRDB.

k8s-ci-robot added the do-not-merge/work-in-progress label Jan 25, 2024

pav-kv force-pushed the refactor-send-append branch 7 times, most recently from eeefed5 to 9cc9344 Compare January 25, 2024 18:12

pav-kv force-pushed the refactor-send-append branch from 9cc9344 to 580374c Compare February 8, 2024 11:41

pav-kv changed the title ~~raft: refactor maybeSendAppend~~ raft: consolidate all append message sending Feb 8, 2024

pav-kv force-pushed the refactor-send-append branch from 580374c to b4da150 Compare February 8, 2024 11:59

nvanbenschoten reviewed Feb 13, 2024

View reviewed changes

pav-kv force-pushed the refactor-send-append branch 3 times, most recently from 693ed7b to 2249aff Compare February 23, 2024 13:25

pav-kv commented Feb 23, 2024

View reviewed changes

tracker/progress.go Outdated Show resolved Hide resolved

raft.go Outdated Show resolved Hide resolved

pav-kv force-pushed the refactor-send-append branch 2 times, most recently from 5cc41de to 017bdda Compare February 23, 2024 17:16

pav-kv marked this pull request as ready for review February 23, 2024 18:27

k8s-ci-robot removed the do-not-merge/work-in-progress label Feb 23, 2024

pav-kv force-pushed the refactor-send-append branch from 017bdda to 34d67b7 Compare February 26, 2024 13:16

pav-kv mentioned this pull request Feb 26, 2024

tracker: track in-flight commit index #171

Merged

ahrtr reviewed Feb 26, 2024

View reviewed changes

pav-kv force-pushed the refactor-send-append branch 2 times, most recently from 1a2c70f to 3a94256 Compare February 27, 2024 19:42

pav-kv force-pushed the refactor-send-append branch from 3a94256 to 3f9b634 Compare March 6, 2024 14:52

pav-kv added 3 commits March 11, 2024 11:37

tracker: throttle empty probes

863ff68

Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

tracker: rename the paused probes flow field

91981c3

Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

pav-kv force-pushed the refactor-send-append branch from 3f9b634 to 91981c3 Compare March 11, 2024 11:37

pav-kv mentioned this pull request May 12, 2024

raft: consolidate all append message sending cockroachdb/cockroach#124006

Merged

pav-kv closed this May 29, 2024

pav-kv deleted the refactor-send-append branch May 29, 2024 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raft: consolidate all append message sending #134

raft: consolidate all append message sending #134

pav-kv commented Jan 25, 2024 •

edited

nvanbenschoten Feb 13, 2024

pav-kv Feb 13, 2024 •

edited

pav-kv commented Feb 26, 2024

bdarnell commented Feb 26, 2024

ahrtr commented Feb 26, 2024

pav-kv commented Feb 26, 2024

ahrtr Feb 26, 2024

pav-kv Feb 26, 2024 •

edited

ahrtr Feb 26, 2024

pav-kv Feb 26, 2024 •

edited

pav-kv commented May 29, 2024

		return pr.CanBumpCommit(commit) \|\|
		pr.Match < last && (!pr.MsgAppProbesPaused \|\| pr.CanSendEntries(last))

raft: consolidate all append message sending #134

raft: consolidate all append message sending #134

Conversation

pav-kv commented Jan 25, 2024 • edited

nvanbenschoten Feb 13, 2024

Choose a reason for hiding this comment

pav-kv Feb 13, 2024 • edited

Choose a reason for hiding this comment

pav-kv commented Feb 26, 2024

bdarnell commented Feb 26, 2024

ahrtr commented Feb 26, 2024

pav-kv commented Feb 26, 2024

ahrtr Feb 26, 2024

Choose a reason for hiding this comment

pav-kv Feb 26, 2024 • edited

Choose a reason for hiding this comment

ahrtr Feb 26, 2024

Choose a reason for hiding this comment

pav-kv Feb 26, 2024 • edited

Choose a reason for hiding this comment

pav-kv commented May 29, 2024

pav-kv commented Jan 25, 2024 •

edited

pav-kv Feb 13, 2024 •

edited

pav-kv Feb 26, 2024 •

edited

pav-kv Feb 26, 2024 •

edited