cockroachdb / cockroachdb/cockroach
raft: simplify LogStorage.Entries semantics
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
The [raft.LogStorage](https://github.com/cockroachdb/cockroach/blob/cc494c02ff366c0f8a34d57bbf99422c179e7e2e/pkg/raft/storage.go#L43) stack today includes:
- the log entries [cache](https://github.com/cockroachdb/cockroach/blob/cc494c02ff366c0f8a34d57bbf99422c179e7e2e/pkg/kv/kvserver/raftentry/cache.go#L25)
- [kvserver.replicaLogStorage](https://github.com/cockroachdb/cockroach/blob/cc494c02ff366c0f8a34d57bbf99422c179e7e2e/pkg/kv/kvserver/replica_raftlog.go#L28)
- the in-memory `raftLog/unstable` [structure](https://github.com/cockroachdb/cockroach/blob/cc494c02ff366c0f8a34d57bbf99422c179e7e2e/pkg/raft/log.go#L49) in `raft` package
- the `raft.LogStorageSnapshot` and its implementation
All the parts of this stack implement the same semantics for the [Entries](https://github.com/cockroachdb/cockroach/blob/cc494c02ff366c0f8a34d57bbf99422c179e7e2e/pkg/raft/storage.go#L67) call:
- the returned entry slice never exceeds `maxSize`
- except when the first entry is large, in which case the first entry is returned
- the entry size is computed as `raftpb.Entry.Size()` for the protobuf encoding of the entry
The downsides of this logic:
1. The contract to not return entries above `maxSize` is overly restrictive. The storage interaction today requires reading the entry to compute its size. The entry can then be [dropped](https://github.com/cockroachdb/cockroach/blob/cc494c02ff366c0f8a34d57bbf99422c179e7e2e/pkg/kv/kvserver/logstore/logstore.go#L642-L647) from the returned slice only because it moves the total size above `maxSize`. However, we've already paid the cost of fetching this entry from the storage, so it would be better to return it nevertheless.
2. The protobuf size computation is cheap, but not free.
We should change the semantics:
1. Allow returning the entries that bring the total size above `maxSize`, if they have been already fetched/parsed anyway.
2. Account only for the `len(entry.Data)`, instead of `entry.Size()`, because it is cheaper and good enough [[ref](https://github.com/cockroachdb/cockroach/pull/132600#pullrequestreview-2367632892)]. If the caller of `Entries()` is concerned with empty entries, they should limit the `[begin, end)` range accordingly.
Jira issue: CRDB-43277
Contributor guide
Assessment
This issue has not been assessed yet.