nit: I would return absl::optional here as opposed to 0 to make it more clear when there is no timeout.

I have to look again every time I come back to this, but did you double check that conn pools are always deleted inline and not in deferred delete context? I think this is true and we can delete/disable the idle timer in the destructor but I want to make sure we don't have a potential issue where the timer might fire after the pool has been deferred deleted.

It looks like the HTTP conn pools are never deferred deleted, but unfortunately the TCP conn pools are -- see ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainTcpConnPools.

I can add a cancelPoolIdleTimer() method to ConnectionPool::Instance and add a call to it in drainTcpConnPools, unless there's a better pattern to follow.

I would need to spend more time with the code to give you a better assessment here. It would be nice to have this be self contained somehow and not add more interface methods, but maybe that is not possible. cc @alyssawilk for additional thoughts as she has been in this code more recently.

I think (please check) that drainConnectionsImpl is always called before pool destruction so that might be a good and safe place to unregister.

I put the timer cancellation into closeIdleConnectionsForDrainingPool since that is called from all drain paths.

per other comment I would use absl::optional here.

I don't see this return value used in the prod code. Can you use it even if only asserting it? Otherwise drop it?

assert added where it is used in the prod code.

I would have to dive back in and fully revisit the logic, but it seems to me that checkForDrained and checkForIdle are doing very similar things and have to be called at all the same points. Can they be merged into checkForDrained to limit the required changes in this PR as well as logic divergence?

I considered whether the logic could be merged, but I think there are subtle differences that prevent that (I think).

checkForDrained does the following:

• Closes connections in the READY state (and CONNECTING if there are no pending streams)
• Fires the drained callbacks if there are no pending, ready, busy, and connecting clients

For checkForIdle, we don't want to change the state of the connections at all. I think we also primarily care about active and pending streams rather than the connection state (e.g. if the pool has a bunch of CONNECTING connections but no pending streams, it is still idle).

There are also a few cases where we need to check for idle but we don't want to check for drained, e.g. when a stream closes we want to check whether we just went idle, but it doesn't make sense to check if we've drained since it's unlikely the connection bearing the closed stream has been drained yet.

Does that make sense?

I'm probably missing something, but my general take is that "idle" and "drained" is basically the same thing. We might do different things (fire drained callback or start timer) but I don't quite understand how they are different in that we are checking to see if there are no active connections.

Can we unify into a single function that takes an enum param if that matters?

My general take is it's going to be pretty hard for the next reader to understand how these things are different and we should try to unify them. cc @alyssawilk on this point also.

I added some comments to distinguish between these cases -- I hope it's clearer now.

When a pool is "idle" it means that there may be live connections, but those connections do not have any active streams. If the pool sees activity, it ceases to be idle. When the idle timer fires, the pool is drained and removed from the map.

Prior to this PR, a pool is only put into the "draining" state under (afaik) two conditions: the host is unhealthy or the host is being removed. This adds a third state -- the pool has been idle for the idle timeout duration.

revert

I know we discussed this before, but it's been a while so I don't remember where we landed with this. Conceptually, it feels broken that we have "drain callbacks" as well as "idle callbacks."

Conceptually these seem like the same thing. How is idle different from draining?

Could we model this differently by doing something like the following:

1. Having draining just be a bool state (yes/no).
2. Have idle callbacks and timeout be independently setable.
3. Draining is basically just setting the drain boolean and setting the idle timeout to 0?

cc @alyssawilk who I would also like to look at this since she has been in this code a lot recently.

Just getting back to this...

In this PR, the idle timer starts whenever there are no pending or active clients for the pool. When the timer fires, the pool is drained.

So the sequence is currently:

1. No active or pending clients on the pool, pool idle timer is set
2. Idle timer fires, idle callbacks called
3. Cluster manager drains the pool
4. All connections on pool are closed, drain callbacks called
5. Cluster manager deletes pool

I think in order to make your suggestion work, the definition of the start of the "idle" interval would have to be after all connections have hit their connection idle timeouts. Since "drained" requires that all connections are closed, in order for there to be a single callback, the idle timeout will also have to be fired at that time or later.

This has the downside that a user will have to appropriately configure both the connection idle timeout and the pool idle timeout on a connection pool. Leaving the default connection idle timeout for HTTP would mean that the timeout would be 1 hour + the configured pool idle timeout. If we're okay with asking the user to configure both timeouts, I can make the appropriate changes to this PR and merge the drained callbacks and the idle callbacks.

The new sequence for an idle timeout would be:

1. All connections on the pool hit their idle timeout, pool idle timer is set
2. Pool idle timer fires, idle callbacks called
3. Cluster manager deletes the pool

So I feel strongly that we should merge drain/idle. IMO it's the same thing. But I'm still confused. Isn't "draining" just setting an idle timeout of 0 as it is implemented today? Why can't the explicit drain process just reset idle timeout to 0 regardless of what it was configured for?

The current implementation explicitly closes all connections and waits for the connection count to reach zero before calling the drain callbacks, so it's not exactly setting the connection idle timeout to 0.

In this PR I introduced a pool idle timeout independent of the individual connection timeouts, but I think what you're suggesting is that the connection idle timeouts should be good enough.

If we were to rely on the connection idle timeouts only, I'm not sure what to do with the case where all of the connections in a pool close for reasons other than an idle timeout (e.g. a momentary loss of connectivity with the upstream). If we were to rely solely on the number of active connections reaching zero to determine when the pool is "idle", then we might be prematurely deleting and recreating pools if the connections in the pool error out. Maybe it's fine to delete and recreate pools in that case?

I can go back and look at the code again, but I'm nearly positive that the definition of drained has nothing to do with connection idle timeouts and only whether there are active/pending requests. This was implementing a long, long time ago before there even were connection idle timeouts. Are you sure your analysis is correct on what drain does?

You're right that it has nothing to do with connection idle timeouts -- I thought you were referring to the connection idle timeouts when you said "Isn't 'draining' just setting an idle timeout of 0 as it is implemented today?"

My understanding of the drain process is as follows (in ConnPoolImplBase::drainConnectionsImpl):

1. Close all connections without active requests (closeIdleConnectionsForDrainingPool)
2. Move all busy/ready clients to the DRAINING state
3. After certain events (e.g. request completion) close all connections without active requests
4. When all connections have been closed, call the drained callbacks

My understanding of the code is that a pool isn't drained until it has no pending or active requests and its connections have been closed.

Right but the point is connections are actively closed when they have no active/pending requests. This means we don't wait for connection idle. Given this, I'm pretty sure it's effectively just setting idle timeout to 0?

Yes, I agree that it's effectively setting the connection idle timeout to 0.

So if I'm understanding correctly:

• the idle callbacks should trigger based on the connection idle timeout of the last connection in a pool
• the idle and drained callbacks should both be added via the same API
• starting to drain should be setting a draining boolean

The question I have now is: what should we do about a pool whose connections are closed prior to the connection idle timeout -- for example, the remote endpoint closes all connections? Is it acceptable to tear down the pool at that point, even if that means we might recreate the pool immediately afterwards?

Is it acceptable to tear down the pool at that point, even if that means we might recreate the pool immediately afterwards?

I don't see a problem with this, but if you want you could guard this functionality with whether the draining boolean is set? Up to you.

is_draining |= (drain == Instance::DrainPool::Yes) ?

comment why the is_draining_ check here and not elsewhere please

is has_seen_clients_ valid for the is_draining case?
AFIK if a cluster has never seen traffic and it is told to drain, it should drain.

I wonder if for clusters we're willing to erase we should simply only create them when there's demand, and if the work between creating the cluster and creating a stream would allow removing this boolean?

You're right -- I think we need to drain right away. I'll push an update with a test.

We already create connection pools on demand in ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::{tcpConnPool,connPool} -- the bool is there to prevent an immediate cleanup on a new pool when the first idle callback is added (we call checkForIdle when a callback is added).

We might be able to remove the bool if we don't check for idle in addIdleCallback when DrainPool::No is passed to the function. That would also solve the issue of always draining when being told to. WDYT?

yeah, if it's just there to avoid immediate drain I think we can probably avoid it with careful calls. I'm also fine leaving it in as a "but we want to be paranoid" precaution, with appropriate comments :-)

What is the actual cost of adding a new pool? I'm thinking not that much? And if we only remove a pool in the case of connection idle timeouts or failure of all connections to all hosts, it seems like an edge case and not a huge deal? I think my preference would be to remove this config option, runtime guard the change per your recommendation, but default it to on? @alyssawilk wdyt?