raft: remove apply pacing #133175
raft: remove apply pacing #133175
Conversation
Epic: none Release note: none
Epic: none Release note: none
This commit removes the committed entries application pacing from the raftLog type. This is done for a few reasons: - CRDB does not use this capability. Entries are applied immediately in the Ready handler, so there are no times when two MsgStorageApply messages are in-flight. - If we re-introduce this pacing, it will be done outside the raft package. Instead of having the per-range in-flight limit, we are going to need a node/store level limit, to help avoid OOMs. - We are moving away from doing IO from within the RawNode. Fetching committed entries from log storage will be done via the LogSnapshot at some point. It will be natural for the caller to implement the size policies at its level. - This change separates the concerns of raftLog being a data structure, vs flow control / pacing. Before this change, the two were coupled in the raftLog type. Epic: none Release note: none
|
This change is |
pav-kv
force-pushed
the
raft-rm-apply-limit
branch
from
4cb98c4 to
7d26089
Compare
Epic: none Release note: none
pav-kv
force-pushed
the
raft-rm-apply-limit
branch
from
7d26089 to
190baa0
Compare
There was a problem hiding this comment.
So this code that you deleted would've been useful only in cases in which entry application is delegated to an async thread, right? I'm surprised that you didn't need to remove/change any tests, meaning that there wasn't any coverage. Looks like most of the code you're removing was added in etcd-io/raft@2e0653d.
I agree that we're moving toward a world in which raft communicates what can be applied in principle (i.e. the commit index) rather than doling it out piecemeal, so I'm on board with stripping this functionality down to the bare minimum (don't emit entries to apply unless the previous batch is acked). Is there a reason you didn't strip it further to the point where entries have to be applied before acking the ready?
| @@ -505,8 +505,6 @@ process-ready 1 2 3 | |||
| HardState Term:1 Vote:1 Commit:13 Lead:1 LeadEpoch:1 | |||
| Entries: | |||
| 1/15 EntryNormal "prop_4" | |||
| CommittedEntries: | |||
There was a problem hiding this comment.
I still need to audit it and confirm, but I think: there can only be one MsgStorageApply in flight now, so some commits migrated between Readys. The test probably exercised > 1 message in flight.
Yes. We don't do it today. If we did, we would have a risk of OOM because there is no limit on the total volume of entries in the apply thread's queue across all ranges. So if we ever want an asynchronous append thread, it should come with some store/node-scoped flow control outside
There was coverage. I had to fix ~5 unit tests which were exercising this flow control capability, and also in some cases assumed more than one
Just making a minimal change scoped to the |
| storage: storage, | ||
| unstable: newUnstable(last, logger), | ||
|
|
||
| // Initialize our committed and applied pointers to the time of the last |
This commit removes the committed entries application pacing from the
raftLogtype. This is done for a few reasons:Readyhandler, so there are no times when twoMsgStorageApplymessages are in-flight.raftpackage. Instead of having the per-range in-flight limit, we are going to need a node/store level limit, to help avoid OOMs.RawNode. Fetching committed entries from log storage will be done via theLogSnapshotat some point. It will be natural for the caller to implement the size policies at its level.raftLogbeing a data structure, vs flow control / pacing policies. Before this change, the two were coupled in theraftLogtype.Related to #105850