Add MinIdleConnsHeadroom support

[pracucci]

Currently memcached client allows to configure the max number of idle connections. These idle connections are never closed, even if unused for a long time. There's some tensions on the value you set for idle connections:

• Too low: new connections are opened for each Get operation when your concurrency is > max idle connections
• Too high: a short spike in Get concurrency will open new connections which then are kept idle until the process restarts even if unused

In this PR I'm proposing to add a MinIdleConnsHeadroom config option. When configured, the client periodically closes idle connections up until MinIdleConnsHeadroom percentage of the recently used connections. If MinIdleConnsHeadroom is not configured (zero value) there's no difference compared to previous version.

In order to periodically close the connections, the client now runs a maintenance goroutine. The client goroutine can be stopped calling the new Close() function.

Other the new unit tests, I've manually tested it in Mimir (see grafana/mimir#4249) and looks working as designed.

[colega]

I would consider an alternative logic: define the MinIdleConnsHeadroom as the percentage of conns using the last specified period.

Let me explain myself: consider a 1 minute period, if a connection has been used during that last 1 minute period, then we don't consider it "idle" at all: it is idle in terms of the network, but we know that some load spike during the last minute has used it, so we shouldn't really close it. Let's call these connections used during the last 1 minute "recently used".

Let's say we have 100 active connections, then with MinIdleConnsHeadroom=50 we should leave 50 really-idle connections on top of those 100 recently used, to account for the possible spikes.

If the use decreases, let's say to 50 active conns, then we'll close connections until we have 75 (50 recently used + 25 idle). This will not happen in a single close, as the number of active connections will naturally decrease over time.

I see two advantages in this:

• The number of idle connections we keep depends on the number of used ones, rather than an absolute minimum.
• The logic is (IMO) simpler to implement: take the slice of conns, count the recently used ones, and then skip the percentage of idle we want to keep, and close the rest.

---

This is just an alternative idea to consider, I'm not saying there's something wrong with your logic 👍

[pracucci]

I would consider an alternative logic: define the MinIdleConnsHeadroom as the percentage of conns using the last specified period.

I like your idea because you don't have to think about the absolute minimum value to set. I'm going to change the implementation.

[colega]

LGTM, thanks!

[colega]

Maybe this is easier to follow? Not a strong opinion:

Suggested change

	numIdleToClose := len(freeConnections) - numRecentlyUsed - numIdleToKeep
	numIdleToClose := numIdle - numIdleToKeep

[pracucci]

👍 Done in c873d2f.

[colega]

Why MinIdleConnsHeadroom can't be >= 1? It's valid to want to have twice as much connections as the recently used ones. Doesn't make much sense, but it's valid.

[pracucci]

Reason was that to me didn't make much sense, but doesn't hurt to support it. Done in 4413597.

[colega]

Why do we need to keep at least 1 idle connection? You already have the number of recently used ones, you may not want to have any extra ones.

Which implies: should we allow a minIdleHeadroom=0 here? I think we should.

[pracucci]

Agree. Done in 4413597.

[colega]

Left some opinionated comments, they're not blockers, but consider thinking about them.

[pracucci]

Thanks @colega for your review. I should have addressed all comments, plus added few more test cases and made Close() idempotent. Can you take another look, please?

[colega]

Looks awesome! Thank you!