Add some jitter for avoiding purge more than one idle/stale connections at once

[pfreixes]

We have observed that the current pool implementation is very eager on removing idle connections and the stale ones [1]. In some situations we have seen that all idle operations can be drop all or a significant amount at the same time, same for the stale ones. This impacts directly on the latency of the requests, since new inflight requests might be still requiring a portion of the connections that where drop.

We would like to propose the addition of a safe guard or a jitter time for avoiding this kind of situations, which will guarantee that the drop of the connections is done progressively and when there is confirmation that they are no longer required, for keeping compatibility we could introduce a new Pool config option called minConDropLag, for making sure that at most 1 connection is dropped - because of timing out the idle time or the stale one - within a minConDropLag time window. This value could be set to -1 by default meaning not being effective, while 0 could mean a default value like 60 seconds.

Thoughts?

[1] https://github.com/go-redis/redis/blob/master/internal/pool/pool.go#L266

[vmihailenco]

This does not look important enough to introduce a configuration option, but perhaps we can make this a default behavior, e.g. only drop half of the idle conns.

Also, we could try to remove periodic Pool.reaper altogether and instead add MaxIdleConns (similar to database/sql) so idle conns are closed when they exceed certain limit. Will that help with your problem?

---

Do you have any charts/numbers to share how this affect your program? We don't want to further complicate the pool without a good reason so it would be nice to understand how important this problem is...

[pfreixes]

This does not look important enough to introduce a configuration option, but perhaps we can make this a default behavior, e.g. only drop half of the idle conns

Ive just wanted to provide a configuration option for not breaking the original behaviour since others might have the expectations that after a spike of traffic idle connections will be purged "immediately", if we add a "lag" this will no longer happen and downscale would happen gradually.

Not sure if half of the idle would be to much ... Ill try to investigate what other pools are doing

Also, we could try to remove periodic Pool.reaper altogether and instead add MaxIdleConns (similar to database/sql) so idle conns are closed when they exceed certain limit. Will that help with your problem?

We have for now configured the pool with MaxIdleConns for forcing to have a preallocated set of connections and disabled the idle check frequency and idle timeout, this seems to work - thought we are still experimenting some weird scenarios, but let me dig more into this.

Do you have any charts/numbers to share how this affect your program? We don't want to further complicate the pool without a good reason so it would be nice to understand how important this problem is...

Good point, Ill share some charts