storage: Track renewable expiration-based leases in map instead of chan#28975
storage: Track renewable expiration-based leases in map instead of chan#28975craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
As discussed on cockroachdb#28931 Release note: None
|
ping @petermattis |
petermattis
left a comment
There was a problem hiding this comment.
Thanks for the ping. Somehow missed this.
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/storage/store.go, line 432 at r1 (raw file):
// renew. An object is sent on the signal whenever a new entry is added to // the map. renewableLeases syncutil.IntMap // map[roachpb.RangeID]*Replica
I think syncutil.IntMap might be inappropriate here due to the usage pattern. syncutil.IntMap should be used for read-mostly maps. I think a regular map with a mutex is likely better.
On the other hand, there would definitely be more awkwardness with a regular map and a mutex. And the renewableLeases map is stable until a lease changes hands. Ok, I'm fine with this as-is.
a-robinson
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/storage/store.go, line 432 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
I think
syncutil.IntMapmight be inappropriate here due to the usage pattern.syncutil.IntMapshould be used for read-mostly maps. I think a regular map with a mutex is likely better.On the other hand, there would definitely be more awkwardness with a regular map and a mutex. And the
renewableLeasesmap is stable until a lease changes hands. Ok, I'm fine with this as-is.
I chose the sync.Map because it means that insertion and iteration don't interfere with each other. Alternatively we'd have to copy the entire map in each iteration of the loop in startLeaseRenewer in order to avoid holding the lock while calling redirectOnOrAcquireLease on each replica.
And as you mention, this map should actually be read-mostly because writes only occur when a lease changes hands.
|
bors r+ |
Build failed (retrying...) |
28912: kv: teach DistSender about RangeFeeds, use for changefeeds r=nvanbenschoten a=nvanbenschoten This change adds a RangeFeed method to DistSender and teaches it how to split rangefeeds across ranges and how to manage their individual lifecycle events. It then adjusts the changefeed poller to use RangeFeed requests when the corresponding cluster setting is enabled. 28975: storage: Track renewable expiration-based leases in map instead of chan r=a-robinson a=a-robinson As discussed on #28931 Release note: None Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com> Co-authored-by: Alex Robinson <alexdwanerobinson@gmail.com>
Build succeeded |
As discussed on #28931
Release note: None