kvserver: read lease under mutex when switching lease type #124223
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
kvoli
force-pushed
the
240513.consistent-lease-status-for-switch
branch
from
cab4b67
to
d08fe81
Compare
|
This passes 10/10 tests for |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @kvoli)
pkg/kv/kvserver/replica_range_lease.go line 1621 at r1 (raw file):
defer r.mu.Unlock() st := r.leaseStatusAtRLocked(ctx, r.store.Clock().NowAsClockTimestamp())
We should pull the r.store.Clock().NowAsClockTimestamp() outside of the replica mutex.
pkg/kv/kvserver/replica_range_lease.go line 1627 at r1 (raw file):
if !r.hasCorrectLeaseTypeRLocked(st.Lease) { return r.requestLeaseLocked(ctx, st, nil /* limiter */)
ignorable nit: consider structuring this to separate the no-op cases from the switch lease case:
if !st.OwnedBy(r.store.StoreID()) {
return nil
}
if r.hasCorrectLeaseTypeRLocked(st.Lease) {
return nil
}
return r.requestLeaseLocked(ctx, st, nil /* limiter */)
A race could occur when a replica queue and post lease application both attempted to switch the lease type. This race would cause the queue to not process the replica because the lease type had already changed. As a result, lease preference violations might not have been quickly resolved by the lease queue. Read the lease under the same mutex used for requesting the lease, when possibly switching the lease type. Resolves: cockroachdb#123998 Release note: None
kvoli
force-pushed
the
240513.consistent-lease-status-for-switch
branch
from
d08fe81
to
574f667
Compare
There was a problem hiding this comment.
TYFTR!
Reviewable status:
pkg/kv/kvserver/replica_range_lease.go line 1621 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
We should pull the
r.store.Clock().NowAsClockTimestamp()outside of the replica mutex.
Done.
pkg/kv/kvserver/replica_range_lease.go line 1627 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
ignorable nit: consider structuring this to separate the no-op cases from the switch lease case:
if !st.OwnedBy(r.store.StoreID()) { return nil } if r.hasCorrectLeaseTypeRLocked(st.Lease) { return nil } return r.requestLeaseLocked(ctx, st, nil /* limiter */)
This is easier to follow, updated.
|
bors r=nvanbenschoten |
A race could occur when a replica queue and post lease application both attempted to switch the lease type. This race would cause the queue to not process the replica because the lease type had already changed. As a result, lease preference violations might not have been quickly resolved by the lease queue.
Read the lease under the same mutex used for requesting the lease, when possibly switching the lease type.
Resolves: #123998
Release note: None