cockroachdb / cockroachdb/cockroach
raft: make the API type-safe
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
The `raft` API combines all flows into one interface:
- [Message](https://github.com/cockroachdb/cockroach/blob/e6d22a8ffb875d579513fd23fff955ccc495e95f/pkg/raft/raftpb/raft.proto#L72-L124) is a union of all possible messages (local and remote)
- One [Step](https://github.com/cockroachdb/cockroach/blob/e6d22a8ffb875d579513fd23fff955ccc495e95f/pkg/raft/raft.go#L1041) for putting messages in
- One [Ready](https://github.com/cockroachdb/cockroach/blob/e6d22a8ffb875d579513fd23fff955ccc495e95f/pkg/raft/rawnode.go#L130) for getting anything out of it
(NB: this uniformity is already not used in a few places, e.g. we have methods like `ProposeConfChange`, `ApplyConfChange`, `ReportUnreachable`, `ReportSnapshot` that fall out of this pattern; we’ll have more of these with Replication Admission Control)
The downsides of this API spread a long way into the CRDB code. We often use reasoning like:
- When _something_ happens, raft will send us a `MsgAppResp`; from the `MsgAppResp`, we infer the durable log index.
- Or: when we receive a `MsgStorageAppend`, it will contain `Responses` containing `MsgStorageAppendResp` and `MsgAppResp` to other nodes - we infer some information from that.
- Or: when we step a `MsgProp`, raft will populate index/term in the `Entries`, so we can use this side effect to understand where this append landed in the log.
There is a lot of unsafe switch-casing based on message types, and breaking the raft’s abstractions.
I think it would be cleaner if all exchanges with `raft.RawNode` were strongly typed methods, not messages, and the responsibility of packing/unpacking these to messages would be on the upstream code.
Raft is a collection of multiple semi-independent state machines. There is one for interaction with the local storage, one for interaction with every follower/peer (the `MsgApp` flows), and more. Each of these state machines can be “ready” at different times. Also, each of these state machines effectively has its own step/ready API and its own message types. For example:
- Local storage state machine is “ready” when it has anything to send to the storage.
- So we can have an individual ready handling flow for it.
- This state machine sends a few things to storage, and will receive a response later.
- Instead of multiplexing this behaviour into the union of all behaviours in `Step/Ready/Message`, we can have a `Ready`-style method that returns a concrete type encapsulating this state machine’s updates, and have a `Step`-style method with concrete types that puts the results back.
- E.g. `GetLogWrite` and `StepLogWriteResponse` methods would do.
- `HasReady` becomes a bitmask / descriptor of all the pieces that are ready. It is input into the scheduler.
This can translate to raft scheduler having one “ready” handler per sub-state-machine. This allows scheduling individual pieces of `Ready` processing independently. In many cases, it is cheaper than currently: today `Ready` is a [disjunction](https://github.com/cockroachdb/cockroach/blob/62e90edbe21d0afe9dfb26c216ba91730e74ee43/pkg/raft/rawnode.go#L433-L453) of all individual ready-s; we have to [compute](https://github.com/cockroachdb/cockroach/blob/62e90edbe21d0afe9dfb26c216ba91730e74ee43/pkg/raft/rawnode.go#L128-L175) this disjunction at all times, and `handleRaftReady` also needs to check every part of the `Ready` struct to be non-empty, so is also a disjunction of all Ready-s.
Benefits of breaking the API into flows:
- There is a finer grained control over the execution. All of the “heavy” flows (log storage, apply, `MsgApp`) can be throttled by the upper layer and be subject to AC / memory budgeting etc.
- “Urgent” messages (like voting, heartbeats/liveness, etc) can be stepped eagerly instead of being queued with all other messages.
- Most/all of the sub-state-machines of raft can compute the “ready” state/message on the fly. This would eliminate the need for slices of abstract `raftpb.Message`, and “compact” all the messages, pertaining to one sub-state-machine, that often [cancel each other out](https://github.com/cockroachdb/cockroach/blob/34a97d3842e8d50ba89bed97d0aa76845570cd60/pkg/kv/kvserver/replica_raft.go#L1717-L1724), into a single struct per ready iteration. For example, instead of generating a `MsgApp` eagerly on every append, we can compute one up-to-date append request at ready handling time; similarly, stale vote requests are not interesting, we only need to send the latest ones for the current `Term`.
- Strong types. There is no “union of all messages” and “union of all behaviours”.
- This makes it easier to reason about things like: what is the full list of places where we can step a message of a particular type, or what is the lifetime of a particular message type? Browse for all method calls corresponding to this event and where this type is used.
- No dependency on `raftpb.Message`, and eventually the library can have no dependency on `protobuf` (which allows swapping to other wire protocols if we want to).
- The serialization / switch-casing based on message types would be consolidated in a few places, somewhere close to `RaftTransport`. All other code would be strongly typed with cleaner semantics.
Jira issue: CRDB-39559
Contributor guide
Assessment
This issue has not been assessed yet.