Add minSize to pool to maintain a minimum # of connections #1319
Conversation
| if (needed > 0) { | ||
| ContextInternal context = vertx.getOrCreateContext(); | ||
| for (int i = 0; i < needed; ++i) { | ||
| acquire(context, connectionTimeout, (ar) -> { |
There was a problem hiding this comment.
I'm wondering if there should be a flavor of acquire that forces new connection creation. Thinking of the following scenario:
If the maxPoolSize is 16, minSize is 8, and let's say steady state connection usage is 4 (so pool.size() will be approximately 4). In this case, is it possible that some/all needed calls to acquire (i.e. needed = 8 - 4 = 4) end up getting one or more of the steady state connections when they are not in use, and as a result, < 4 (or even 0) new connections get created, and pool.size() doesn't reach close to 8 (or worse, remains at 4)?
@tsegismont is this a valid concern?
There was a problem hiding this comment.
Over time (e.g a period of 10 - 30 seconds) this brings up the required number of connections and keeps it there.
Even though I currently don't see this behavior your example and concern may be valid. There is a simpler fix though, just pass the required minimum to the evict function; it can easily stop the eviction test when the minimum would be breached.
There was a problem hiding this comment.
This requires a change to vert.x core.
There was a problem hiding this comment.
I've updated the PR with a simpler fix. Instead of calculating a needed value, it now checks if the pool is "low" and acquires minSize connections immediately (in parallel).
This seems to work pretty well and ensures that minSize will be honored at all times. The downside is that it may start more than minSize connections depending on if any of the connections are in use.
In practice this "downside" doesn't appear that often. Remember that to trigger the acquisition of connections the pool has to be in some kind of "idle" state because connections are idling and falling below the pool minimum. Worst case a few extra connections are activated for a short period of before idling away.
In the end the implementation is somewhere between a minSize and minAvailable depending on the state of the pool and I'm pretty ok with that.
One thing to remember when discussing this is that the pool does wait a beat to see if a connection becomes available; it doesn't immediately start a connection when an acquisition request cannot be immediately fulfilled.
Looking back to your scenario of current pool size of 4 and a minSize of 8. The pool wold attempt to acquire 8 connections. In the case where all 4 connections are in-use, the pool would attempt to acquire 8 connections, potentially bumping it to 12 connections; but only if none of the connections are recycled while the pool is acquiring the connections.
There was a problem hiding this comment.
Thanks, I think this should work for most cases.
There was a problem hiding this comment.
Because of the asynchronous nature of acquire, we shall wait for all operations to complete before recycling the lease. Otherwise, there is no guarantee that some of the operations will complete before all operations are submitted.
|
Hi @tsegismont - this change looks good to me. Do you think someone more familiar with this code can approve it? Thank you. |
|
@tsegismont is out until May 2nd @vietj you have any thoughts on this one? |
vertx-sql-client/src/main/java/io/vertx/sqlclient/impl/pool/SqlConnectionPool.java
Outdated
Show resolved
Hide resolved
vertx-sql-client/src/main/java/io/vertx/sqlclient/impl/pool/SqlConnectionPool.java
Outdated
Show resolved
Hide resolved
|
Hi, @vietj - any thoughts on this PR? I think it looks ready to merge. Thanks! |
|
Hi @tsegismont - any further thoughts on this PR? |
There was a problem hiding this comment.
Thank you @kdubb
In addition to the comment inline:
- please add a test for this new feature
- I think we shall check invariants immediately after the pool is created, otherwise, we have to wait for the task to be scheduled before getting
minSizeconnections.
| if (needed > 0) { | ||
| ContextInternal context = vertx.getOrCreateContext(); | ||
| for (int i = 0; i < needed; ++i) { | ||
| acquire(context, connectionTimeout, (ar) -> { |
There was a problem hiding this comment.
Because of the asynchronous nature of acquire, we shall wait for all operations to complete before recycling the lease. Otherwise, there is no guarantee that some of the operations will complete before all operations are submitted.
|
I am wondering if that should not be a feature that is directly in the pool instead of being specific to this implementation |
| } | ||
| }); | ||
| } | ||
|
|
||
| public void checkMin(long connectionTimeout) { | ||
| if (pool.size() < minSize) { | ||
| ContextInternal context = vertx.getOrCreateContext(); |
There was a problem hiding this comment.
I think the issue here is that we might create connections with a context that is not best for the application and force context switching
There was a problem hiding this comment.
I assume you're referring to vertx.getOrCreateContext(). How would you solve this? Pass in a context?
There was a problem hiding this comment.
Hi, @tsegismont @vietj, any comments?
|
Is this feature still considered for inclusion ? |
|
@tmonney yes it is considered, I haven't looked back at this PR but according to my comments, I had doubts about the actual PR |
Motivation:
Adds a
minSizeparameter to pool options to ensure a minimum # of connection is always connected and available.Conformance:
You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines