storage: return one entry less in Entries #29579
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/storage/entry_cache.go, line 196 at r1 (raw file):
ent := v.(*raftpb.Entry) size := uint64(ent.Size()) if maxBytes > 0 && bytes+size > maxBytes {
Did you consider an alternative approach to add a truncateEntries() function which takes a slice of entries, lo and hi indexes and a maxSize setting and duplicates the etcd/raft semantics? That seems like it would avoid a change here and could be more easily tested.
There was a problem hiding this comment.
Reviewed 5 of 6 files at r1.
Reviewable status:
pkg/storage/entry_cache.go, line 181 at r1 (raw file):
func (rec *raftEntryCache) getEntries( ents []raftpb.Entry, rangeID roachpb.RangeID, lo, hi, maxBytes uint64, ) (_ []raftpb.Entry, size uint64, nextIndex uint64, hitMaxSize bool) {
s/hitMaxSize/exceededMaxBytes/
pkg/storage/entry_cache.go, line 196 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Did you consider an alternative approach to add a
truncateEntries()function which takes a slice of entries,loandhiindexes and amaxSizesetting and duplicates the etcd/raft semantics? That seems like it would avoid a change here and could be more easily tested.
If we did that, we'd need to make sure we call it twice so that we can short-circuit before hitting RocksDB in the happy-path. If we're doing that then it doesn't seem to buy us much.
pkg/storage/replica_raftstorage.go, line 162 at r1 (raw file):
// Note that we track the size of proposals with payloads inlined. size += uint64(ent.Size()) if maxBytes > 0 && size > maxBytes {
What does it mean for maxBytes == 0? I thought Raft uses math.MaxUint64 when it doesn't want a limit.
This works around the bug outlined in: etcd-io/etcd#10063 by matching Raft's internal implementation of commit pagination. Once the above PR lands, we can revert this commit (but I assume that it will take a little bit), and I think we should do that because the code hasn't gotten any nicer to look at. Fixes cockroachdb#28918. Release note: None
tbg
force-pushed
the
fix/applied-index-gap
branch
from
a051ed9 to
9d46451
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/storage/entry_cache.go, line 181 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
s/hitMaxSize/exceededMaxBytes/
Done.
pkg/storage/entry_cache.go, line 196 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
If we did that, we'd need to make sure we call it twice so that we can short-circuit before hitting RocksDB in the happy-path. If we're doing that then it doesn't seem to buy us much.
I did consider it, but as Nathan pointed out it didn't look any better.
pkg/storage/replica_raftstorage.go, line 162 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
What does it mean for
maxBytes == 0? I thought Raft usesmath.MaxUint64when it doesn't want a limit.
I think this was just crufty. Removed.
There was a problem hiding this comment.
Reviewable status:
pkg/storage/entry_cache.go, line 196 at r1 (raw file):
Previously, tschottdorf (Tobias Schottdorf) wrote…
I did consider it, but as Nathan pointed out it didn't look any better.
Right, we'd have to call it twice, but we'd ensure that the same logic is used in both places and could test it in isolation. I don't feel strongly about this and you're already testing both logic paths, so carry on.
|
bors r=petermattis |
Build failed (retrying...) |
29579: storage: return one entry less in Entries r=petermattis a=tschottdorf This works around the bug outlined in: etcd-io/etcd#10063 by matching Raft's internal implementation of commit pagination. Once the above PR lands, we can revert this commit (but I assume that it will take a little bit), and I think we should do that because the code hasn't gotten any nicer to look at. Fixes #28918. Release note: None 29631: cli: handle merged range descriptors in debug keys r=petermattis a=tschottdorf Noticed during #29252. Release note: None Co-authored-by: Tobias Schottdorf <[email protected]>
There was a problem hiding this comment.
Reviewable status:
Build succeeded |
This works around the bug outlined in:
etcd-io/etcd#10063
by matching Raft's internal implementation of commit pagination.
Once the above PR lands, we can revert this commit (but I assume
that it will take a little bit), and I think we should do that
because the code hasn't gotten any nicer to look at.
Fixes #28918.
Release note: None