storage: Deflake TestReplicaReproposalWithNewLeaseIndexError #40477
Conversation
|
This change is |
|
Other than the specific test, we only really need this for an error when marshaling the command footer ( |
There was a problem hiding this comment.
Nice job tracking this down. Failing to insert into the buffer when we reserved an index in the array seems like an issue. Yet, I share your concern about the fix here. Subtracting from the counter does feel pretty heavy-handed. You had to be careful about only subtracting from the array index and not the lease index offset, which I suspect was because doing so would violate strict monotonicity and allow duplicate max lease index assignments. I'm also worried about this causing deadlocks in handleCounterRequestRLocked if it ever allowed zero or more than one request to hit the "The buffer is full and we were the first request to notice" path. I haven't been able to convince myself that that's actually an issue though.
I'm wondering if it would have been sufficient to just ignore empty array elements when flushing the buffer. Would that work to address the cancellation path after a proposal has acquired an array index?
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)
irfansharif
force-pushed
the
proposer-buf-nil
branch
from
668c263
to
7508bd5
Compare
There was a problem hiding this comment.
Doing a nil check when iterating over the buffer works just fine, I only didn't earlier because it seemed smellier but this ended up being worse. I've reverted to that now, PTAL (also alleviates any worries around handleCounterRequestRLocked deadlocking). I kept the tests.
Reviewable status:
irfansharif
force-pushed
the
proposer-buf-nil
branch
from
7508bd5
to
267cb6f
Compare
There was a problem hiding this comment.
.eg.go files.
Reviewed 3 of 3 files at r1, 1 of 9 files at r2.
Reviewable status:
pkg/storage/replica_proposal_buf_test.go, line 273 at r1 (raw file):
b.Init(&p) num := propBufArrayMinSize
nit: any reason to keep this variable around? It would be worth it if you defined num := propBufArrayMinSize / 2.
irfansharif
force-pushed
the
proposer-buf-nil
branch
2 times, most recently
from
054ef0a
to
e94d515
Compare
There was a problem hiding this comment.
woops, TFTR.
Reviewable status:
Fixes cockroachdb#39739. When inserting a new command into the proposal buffer, we first reserve an index into the buffer's array. If we subsequently fail to insert the proposal, we should account for this during proposal registration. Additionally add `TestPropBufCnt` to test basic behaviour of the counter maintained by the proposal buffer. Release note: None
irfansharif
force-pushed
the
proposer-buf-nil
branch
from
e94d515
to
a84f0f4
Compare
|
bors r+ |
40477: storage: Deflake TestReplicaReproposalWithNewLeaseIndexError r=irfansharif a=irfansharif Fixes #39739. When inserting a new command into the proposal buffer, we first reserve an index into the buffer's array and an offset from the buffer's base lease index. If we subsequently fail to insert the proposal, we should undo the index and offset reservation. When we error out in `(*propBuf).Insert`, if we don't undo the index reservation we expect a proposal at said index during consumption, but don't find one. Release note: None Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
Build succeeded |
Fixes #39739. When inserting a new command into the proposal buffer, we
first reserve an index into the buffer's array and an offset from the
buffer's base lease index. If we subsequently fail to insert the
proposal, we should undo the index and offset reservation.
When we error out in
(*propBuf).Insert, if we don't undo the indexreservation we expect a proposal at said index during consumption, but
don't find one.
Release note: None