fix: reduce SetAddrs shards lock contention
#2292
Conversation
|
FYI @szuecs |
ring.go
Outdated
| @@ -213,6 +213,7 @@ func (shard *ringShard) Vote(up bool) bool { | |||
| type ringSharding struct { | |||
| opt *RingOptions | |||
|
|
|||
| addrsMu sync.Mutex | |||
There was a problem hiding this comment.
Please try to make it clear what this mutex protects, e.g. I would expect something like
addrsMu sync.Mutex
addrs type
// another groupThere was a problem hiding this comment.
After reading the code and the PR comment, I suggest to rename this to setAddrsMu and add an explanation similar to the one you have in the PR description.
Neat trick 👍 but please try to add some comments to the code. Without your explanation it is hard to understand what is happening. |
Introduces a new lock to make `SetAddrs` calls exclusive. This allows release of shards lock for the duration of potentially long `newRingShards` call. `TestRingSetAddrsContention` observes number of pings increased from <1000 to ~40_000. See redis#2190 (comment) Updates redis#2077
AlexanderYastrebov
force-pushed
the
ring/reduce-set-addrs-shards-locking-2
branch
from
c117bb2 to
7c6f677
Compare
|
I've addressed comments and added shard cleanup tests. |
| c.mu.Lock() | ||
| if c.closed { | ||
| cleanup(created) | ||
| c.mu.Unlock() |
There was a problem hiding this comment.
Just wonder if we can call cleanup(created) after the mutex is released?
There was a problem hiding this comment.
I thought about it but decided to keep like that - there would not be much difference as the ring is closed anyway at this point. I can move the unlock for aesthetics though.
|
Thanks |
AlexanderYastrebov
deleted the
ring/reduce-set-addrs-shards-locking-2
branch
Introduces a new lock to make
SetAddrscalls exclusive.This allows release of shards lock for the duration of potentially long
newRingShardscall.TestRingSetAddrsContentionobserves number of pings increased from <1000 to ~40_000.See #2190 (comment)
Updates #2077