feature: dynamically add and remove shards by the ring client

[szuecs]

fix #2077 feature: ring.SetAddrs to add and remove shards by the ring client #2093 tests: to scale in and out

Signed-off-by: Sandor Szücs [email protected]

[vmihailenco]

The code could be cleaner if you restructure it like this:

shards := newRingShards(addrs, c.shards)
c.mu.Lock()
if c.closed {
    // close shards
} else {
    c.shards = shards
    c.list = shards.list()
}
c.mu.Unlock()

What do you think?

[szuecs]

@vmihailenco so you are suggesting a type change of c.shards to be able to add a .list() ? I think the problem will be that we would need to lock this .list() and here we don't need it. We could also add an intermediate type, but then we would do:

shards := newRingShards(addrs, c.shards)
c.mu.Lock()
if c.closed {
    // close shards
} else {
    c.shards = shards.shards() // returns map[string]*ringShard
    c.list = shards.list()
}
c.mu.Unlock()

[vmihailenco]

@vmihailenco so you are suggesting a type change of c.shards to be able to add a .list() ?

I suggest to slightly refactor ringShards type so we can fully replace it in SetAddrs holding the lock:

// SetAddrs is safe to be used concurrently.
func (c *Ring) SetAddrs(addrs map[string]string) {
    // Construct new shards without holding the lock. Pass old shards to reuse existing clients.
    shards := newRingShards(addrs, c.shards)

    c.shardsMu.Lock()
    defer c.shardsMu.Unlock()

    if c.closed {
        // Ring is closed. Close new shards.
        shards.Close()
        return
    }

    // Atomically set new shards so they are used by the Ring.
    c.shards = shards
}

Does that make sense?

[szuecs]

@vmihailenco yeah thanks, makes a lot of sense!

[AlexanderYastrebov]

I suggest to slightly refactor ringShards type so we can fully replace it in SetAddrs holding the lock:

This does not look to be safe for concurrent use: imagine two threads call SetAddrs, each creates a new shards and then consequently they take a lock and update c.shards - the second thread would overwrite c.shards and thus leak shards created by the first thread.

[AlexanderYastrebov]

As an idea we may have another mutex to guard only SetAddrs and Close (need to make sure to lock it first in both methods https://stackoverflow.com/questions/38489190/holding-two-mutex-locks-at-the-same-time). This way we ensure no two threads execute SetAddrs/Close while address change is in progress. SetAddrs would only check c.closed once and thus will not need to cleanup newly created shards.

[vmihailenco]

As an idea we may have another mutex

Yes, that is what I suggest as well.

[szuecs]

@AlexanderYastrebov great idea, I hope this is now good, but also happy to rewrite. :)

[szuecs]

@vmihailenco if you give it another try for a review would be great. I had an online session with @AlexanderYastrebov and rewrote some parts to make it nicer.

In an unrelated follow up PR I would like to enhance the locking in ring.go and for example remove the current rebalance()