cockroachdb / cockroachdb/cockroach

raft: simplify LogStorage.Entries semantics

Open
#132,789 0 comments 0 reactions 0 assignees View on GitHub
A-kv-replication C-enhancement C-performance
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.