raft: remove acceptReady from RawNode

[pav-kv]

This PR removes the possibility of getting Ready from RawNode and not accepting it. This API existed to serve concurrency concerns in the implementation of Node interface, which was removed in #131746.

Removing the "accept" phase from RawNode allows further cleanups in the supporting data structures (raftLog / unstable / etc) down the stack.

Conceptually, we do want separation between "get ready" and "accept ready", but in a different form:

• The "get ready" should return general information about the outstanding updates. In the future, this role should be taken by the HasReady() method or its variants (specific to a part of RawNode, like the log storage). Today it only returns one "overall" bit.
• The "accept ready" should be more granular / controllable in terms of how much work the app is willing to commit to. We are gradually moving in this direction, with methods like LogSnapshot() and SendMsgApp(). Today, the Ready() still returns all the work eagerly.

Related to #122131

[cockroach-teamcity]

This change is 

[tbg]

Thanks for the clean up!
The second commit is a little difficult to review - a lot moved and changed at the same time. It would likely have been helpful to split it up: one commit that moves acceptReady right underneath readyWithoutAccept, then a second commit that merges them with only minimal changes, then a third commit that does the simplifications that seem to have happened.
I've reviewed the new code but without checking that it's functionally identical to the previous one.

[pav-kv]

@tbg I split the last commit into 3 as you suggested.

[pav-kv]

TestCommitPaginationWithAsyncStorageWrites fails on CI, though I wasn't able to repro with 20k stress runs. Don't know if it's related to the change (this should be a no-op), will audit it again.

[pav-kv]

Need to double-check that this field is handled equivalently. Previously it was cleared between readyWithoutAccept and acceptReady, now (in the merged code) it is cleared at the end.

[tbg]

TestCommitPaginationWithAsyncStorageWrites fails on CI, though I wasn't able to repro with 20k stress runs. Don't know if it's related to the change (this should be a no-op), will audit it again.

Hmm, seems too suspiciously related. I'll also take a look tmrw.

[tbg]

Looks good to me - approving from a review point of view after your pending comments are addressed.
The test failure is suspicious.

[tbg]

Actually, pattern matching suggests that a flake like this makes some sense. Previously node would "grow" the readies more than now (where each ready is accepted). I think the diff had a comment about "this making tests more stable". Without having looked at the details, it's not too surprising that a change in node timings/internal semantics causes a flake in a node test.

[pav-kv]

The test is inherently flaky. In this bit

cockroach/pkg/raft/node_test.go

Lines 926 to 950 in 7571549

	// Third entry should not be returned to be applied until first entry's
	// application is acknowledged.
	drain := true
	for drain {
	select {
	case rd := <-n.Ready():
	for _, m := range rd.Messages {
	require.NotEqual(t, raftpb.MsgStorageApply, m.Type, "unexpected message: %v", m)
	}
	case <-time.After(10 * time.Millisecond):
	drain = false
	}
	}
	// Acknowledged first entry application.
	require.NoError(t, n.Step(ctx, applyResps[0]))
	applyResps = applyResps[1:]
	// Third entry now returned for application.
	rd = readyWithTimeout(n)
	require.Len(t, rd.Messages, 1)
	m = rd.Messages[0]
	require.Equal(t, raftpb.MsgStorageApply, m.Type)
	require.Len(t, m.Entries, 1)
	applyResps = append(applyResps, m.Responses[0])

Normally, the "drain" loop will see at least one Ready which moves the commit index to 4 (with a MsgStorageAppend). Then we expect that the next Ready returns the last entry 4 for application. So the expected Ready after the "drain" is:

raft2024/09/10 10:42:15 INFO: emitted ready: Ready MustSync=false:
CommittedEntries:
1/4 EntryNormal "aaa"
Messages:
1->ApplyThread MsgStorageApply Term:0 Log:0/0 Entries:[1/4 EntryNormal "aaa"] Responses:[
  ApplyThread->1 MsgStorageApplyResp Term:0 Log:0/0 Entries:[1/4 EntryNormal "aaa"]
]

but we see

raft2024/09/09 12:11:43 INFO: emitted ready: Ready MustSync=true:
HardState Term:1 Vote:1 Commit:4 Lead:1 LeadEpoch:1
CommittedEntries:
1/4 EntryNormal "aaa"
Messages:
1->AppendThread MsgStorageAppend Term:1 Log:0/0 Commit:4 Vote:1 Lead:1 LeadEpoch:1
1->ApplyThread MsgStorageApply Term:0 Log:0/0 Entries:[1/4 EntryNormal "aaa"] Responses:[
  ApplyThread->1 MsgStorageApplyResp Term:0 Log:0/0 Entries:[1/4 EntryNormal "aaa"]
]

That can happen if the "drain" loop gives up too fast (it allows only 10ms). I changed the timeout to 1 microsecond locally, and it reproes the thing (both on master and with this PR). I'll fix this flake in another PR, have an idea.

[pav-kv]

Deflaked the test in this PR, for convenience.

[pav-kv]

Will sequence this PR after #131746, so that tweaking the Node API is not necessary. Could have merged this one first, but neither is urgent.