Feat/set addrs

[vmihailenco]

Closes #2093

[vmihailenco]

@szuecs please take a look at changes I've made. I've removed the 2nd mutex you've added because I doubt it is safe and it does not look like it should affect performance.

[szuecs]

This will block all ongoing traffic through ringshards until it has connected all newRingShards to the connection pools, so we wait until we get all new shards connected until we would serve another ring.$Operation() call.
Example GetByKey is guarded by RLock:
https://github.com/go-redis/redis/blob/c561f3ca7e5cf44ce1f1d3ef30f4a10a9c674c8a/ring.go#L271-L275
which is used in cmdShard https://github.com/go-redis/redis/blob/c561f3ca7e5cf44ce1f1d3ef30f4a10a9c674c8a/ring.go#L573-L581 called by process() https://github.com/go-redis/redis/blob/c561f3ca7e5cf44ce1f1d3ef30f4a10a9c674c8a/ring.go#L592.
So a SetAddrs call will block all Redis ringshard operations until all new shards are connected. In my version it would block all the operations only when we swap the already connected shards. The only shared lock holding which would be blocked are the close operation in my proposed PR.
I care a lot about this, because we actually run multiple clusters with up to 100 redis ring shards and 2-3M operations per second. This will be visible in our latency metrics if we have to stop the processing while connecting new shards.

[vmihailenco]

So a SetAddrs call will block all Redis ringshard operations until all new shards are connected.

newRingShards should not open any connections and should be relatively fast.

In my version it would block all the operations only when we swap the already connected shards.

You are welcome to try to port those changes, but I am not sure it is done correctly or, in other words, this PR can be further optimized.

[szuecs]

newRingShards will create a new connection pool via pool.NewConnPool, which finally runs checkMinIdleConns(), which looks like this:

func (p *ConnPool) checkMinIdleConns() {
	if p.opt.MinIdleConns == 0 {
		return
	}
	for p.poolSize < p.opt.PoolSize && p.idleConnsLen < p.opt.MinIdleConns {
		p.poolSize++
		p.idleConnsLen++

		go func() {
			err := p.addIdleConn()
			if err != nil && err != ErrClosed {
				p.connsMu.Lock()
				p.poolSize--
				p.idleConnsLen--
				p.connsMu.Unlock()
			}
		}()
	}
}

So it starts some goroutines depending on opt.PoolSize and opt.MinIdleConns and calls addIdleConn(), which is creating a new connection via the opt.Dialer.
So it opens new connections, which you have to do anyways. The question is when do you do it, and now you just merged your own PR without discussing it. Honestly a bit odd, but it's your project not mine....

[vmihailenco]

@szuecs okay, go-redis opens new connections only when MinIdleConns is present and still does it asynchronously which should be relatively fast.

now you just merged your own PR without discussing it

Apologies, but I assumed it is better to have something than not having anything at all. We can unexport SetAddrs and continue looking for a solution, but there are not that many volunteers to work on this.

And as I said, I am not sure you PR was ready to be merged either...

[szuecs]

Yeah no problem, I just want to say how I felt.
Let's test the current merged change and if this is an issue we will reach out and propose a change.
Thanks for your collaboration and work on the project!

[vmihailenco]

Thank you for the contribution and for the patience.

[AlexanderYastrebov]

Let's test the current merged change and if this is an issue we will reach out and propose a change.

I agree, lets see how this performs in practice and have a second iteration.

okay, go-redis opens new connections only when MinIdleConns is present and still does it asynchronously which should be relatively fast.

For new shard it is useful to have some established connections before using it.
Asynchronous connection in fact could be a downside - shard would be available for use before there is a working connection, even if we decouple "shard locking for use" and "shard locking for address update".
I think this should be possible to demonstrate by adding sleep() into dialer.