cockroachdb / cockroachdb/cockroach
raft: make the entries encoding maintainable
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
[Entries](https://github.com/cockroachdb/cockroach/blob/5400cb9a70e63bfe1aa2849a566c195ad63130d1/pkg/raft/raftpb/raft.proto#L22-L27) in `raft` contain the user-defined commands in a `[]byte` slice, which our application layer has freedom to define. Today, we have the ever-growing [encodings](https://github.com/cockroachdb/cockroach/blob/f056df4e7109ad52a42dc1a526c22b1da79138df/pkg/kv/kvserver/raftlog/encoding.go#L21-L90) list + ad-hoc entry `entry.Data` parsing [sprinkled](https://github.com/cockroachdb/cockroach/blob/a355dbe389b624d52b91d74ad8801a67958c736d/pkg/kv/kvserver/kvflowcontrol/replica_rac2/processor.go#L917-L926) in random places. This gets harder to maintain and reason about.
The pattern is that the first byte of `Data` contains the "encoding", and we then parse the entry differently, depending on this byte. We often want to sneak peek into the entry's "header" without parsing it entirely, because in most cases it is a protobuf with non-zero unmarshaling cost.
It would be great to replace this ad-hoc encoding with a clean/maintainable solution that allows:
1. Zero/low cost of unmarshaling.
2. Especially in the cases when we only need to check certain things about the entry, e.g. "is this a sideloaded entry?".
One approach to this is replacing the ad-hoc encoding with [flatbuffers](https://flatbuffers.dev/flatbuffers_guide_use_go.html). We would treat the `raftpb.Entry.Data` field as a flatbuffer, with a well-defined / maintainable schema. It would be composed of “header” + data. The header would contain things like:
- “is sideloaded” bit for routing the entries load/store to the right sub-storage
- “sideloaded size”, for size accounting in log truncations stack (at the moment it needs to look at the file to know its size, which is inconvenient and e.g. the decoupled log trunc stack omits sideloaded files accounting in some cases)
- the AC/RACv2 headers
- maybe more
Then all the header checks (like “do we have a RACv2 header?”) would be cheap. As a bonus, we would have no allocations in the entries unmarshaling stack.
As a flip-side, this doesn’t integrate super well with protos, so maybe we would need some wrappers around these flatbuffers to convert to custom types or protos. Though at the scale of just one entries type this doesn’t sound too bad, and is probably better anyway than all the bug-prone parsing we have today. Another flip-side is that this requires a one-time migration that overwrites all raft logs.
However, this logs scan/migration is an opportunity to:
- fixup invariants and remove some old code like [this](https://github.com/cockroachdb/cockroach/blob/a355dbe389b624d52b91d74ad8801a67958c736d/pkg/kv/kvserver/logstore/sideload.go#L205-L215)
- solve problems like #131559
- check some invariants (e.g. indices are contiguous, terms are monotonic, etc)
Jira issue: CRDB-42604
Contributor guide
Assessment
This issue has not been assessed yet.