Configurable test-on-borrow for pooled connections #297
Conversation
Connections are pooled for both direct and routing driver. Each acquired session contains a socket connection which was acquired from the pool. It is possible for an idle pooled connection to become invalid while just resting in the pool. This could happen when connection is terminated after some timeout by a load balancer or some other network facility. This commit introduces a configurable connection liveness check timeout. So freshly acquired connection, that has been idle in the pool for more than configurable timeout, will be validated by sending a RESET message. If it appears to be broken acquisition will retry until a valid connection is found.
lutovich
force-pushed
the
1.1-idle-connection-refresh
branch
from
b3229dc to
d840e07
Compare
| */ | ||
| @Deprecated | ||
| public long idleTimeBeforeConnectionTest() |
There was a problem hiding this comment.
There is no need to keep this old name. Pls pick a good name that better describes this config instead.
| */ | ||
| @Deprecated | ||
| public ConfigBuilder withSessionLivenessCheckTimeout( long timeout ) | ||
| public ConfigBuilder withSessionLivenessCheckTimeout( long value, TimeUnit unit ) |
There was a problem hiding this comment.
Same here, the name should be consistent with the field/method name in Config
|
|
||
| int acquisitionAttempt = 1; | ||
| PooledConnection connection = connectionQueue.acquire( supplier ); | ||
| while ( !canBeAcquired( connection, acquisitionAttempt ) ) |
There was a problem hiding this comment.
Suggesting to test queue.isEmpty() instead. And the while loop would be changed to something like
boolean done = false;
while(!done)
{
if(connectionQueue.isEmpty())
{
done = true; // let `queue.acquire` for queue size + 1 times
}
PooledConnection conn = connectionQueue.acquire(supplier);
if(isHealthyConnection(conn))
{
return conn;
}
}
// `isHealthyConnection` will not throw exception, so this is a final protection of NPE
throw new ClientException("Failed to establish connection with the server or some other better error message");
private boolean isHealthyConneciton(PooledConnection conn)
{
if ( poolSettings.idleTimeBeforeConnectionTestConfigured() && hasBeenIdleForTooLong(conn) )
{
return connectionValidator.isConnected( connection );
}
return true;
}
There was a problem hiding this comment.
Changed code to track when connection was created and then return it. So it'll retry until new connection is created and then return it to the user.
| } | ||
|
|
||
| @Override | ||
| public void close() | ||
| private boolean canBeAcquired( PooledConnection connection, int acquisitionAttempt ) |
There was a problem hiding this comment.
This is not needed if we have isHealthyConneciton (see the previous comment)
| @@ -68,26 +71,41 @@ public SocketConnectionPool( PoolSettings poolSettings, Connector connector, Clo | |||
| public Connection acquire( final BoltServerAddress address ) | |||
| { | |||
| assertNotClosed(); | |||
| BlockingPooledConnectionQueue connectionQueue = pool( address ); | |||
| PooledConnection connection = acquireConnection( address, connectionQueue ); | |||
There was a problem hiding this comment.
Not related to this PR but wonder if it would be better API if we let the pooled connection to hold a reference to connecitonValidator and let the pooledConnection to provide methods to test isConnected and isReusable.
There was a problem hiding this comment.
It could also simplify things around these suppliers and consumers. Let's think about it.
| long idleTimeBeforeConnectionTestMillis = unit.toMillis( value ); | ||
| if ( idleTimeBeforeConnectionTestMillis <= 0 ) | ||
| { | ||
| throw new IllegalArgumentException( String.format( |
There was a problem hiding this comment.
Why not let the user to set to -1 (default value)?
`SocketConnectionPool` can test connections during acquisition. This is done when they have been idle in the pool for too long. This commit makes it retry until new connection is created instead of `maxPoolSize`times.
Renamed `withSessionLivenessCheckTimeout` to `withConnectionLivenessCheckTimeout` because all other methods refer to connections instead of sessions and are actually settings for the connection pool and not session pool.
So feature can be easier turned on and off by users. Zero timeout means always do test-on-borrow, negative timeout means never to test-on-borrow.
|
@zhenlineo comments addressed |
| } | ||
|
|
||
| /** | ||
| * Pooled sessions that have been idle in the pool for longer than this timeout |
There was a problem hiding this comment.
Pls use Pooled connections instead as pooled sessions is a wrong terminology used before.
| @@ -116,17 +123,14 @@ public int maxIdleConnectionPoolSize() | |||
| } | |||
|
|
|||
| /** | |||
| * Pooled connections that have been unused for longer than this timeout will be tested before they are | |||
| * used again, to ensure they are still live. | |||
| * Pooled sessions that have been idle in the pool for longer than this timeout | |||
Changed "session" to "connection" because config should only contain settings for the connection pool. There is no such thing as session pool.
Connections are pooled for both direct and routing driver. Each acquired session contains a socket connection which was acquired from the pool. It is possible for an idle pooled connection to become invalid while just resting in the pool. This could happen when connection is terminated after some timeout by a load balancer or some other network facility.
This PR introduces a configurable connection liveness check timeout. So freshly acquired connection, that has been idle in the pool for more than configurable timeout, will be validated by sending a RESET message. If it appears to be broken acquisition will retry until a valid connection is found.