Add support for minimum connections and auto close idle connections #651
Conversation
|
@sidorares are you interested in landing this? this is making me have to keep a fork of the package to have this feature. I'm hoping we can land this and then encourage mysqljs to also land with same config keys (i don't see why they should always be first) I have some improvements to this in my fork, and will update this PR if your interested in landing it. I don't have the time to work on this for mysqljs :/ |
Generally the same people are members of both repos, at least the top folks :) I have no issue with the name of the config names in this PR and if one were to come to |
Sorry, somehow missed that. I'll try to review as soon as I can
I'd probably have minimum number to be 0, not max. Connect time (for few unlucky users) is small optimisation, while having lots of open connections might be a stress for mysql server |
|
@sidorares if you compare the behavior of mysql2 at this time, once those connections open, they remain open. While I agree a default min of 0 is better, I was suggesting that as a 'no behavior change' approach. But if your ok with a semver minor introducing this albeit harmless behavior change, then I'm fine with that. |
|
I feel it as less breaking. If I have 2 servers running cluster x 8 that's 160 connections immediately on start with this PR, and I think free tier RDS has 100 limit. This means some setups that used to run fine under low load start to crash immediately on start |
|
And semver minor is totally ok as well |
|
I agree with @sidorares 👍 |
lib/pool.js
Outdated
| if (now > conn._lastReleased + timeout) { | ||
| // This connection has been unused for longer than the timeout | ||
| this._extraConnections.shift(); | ||
| conn.destroy(); |
There was a problem hiding this comment.
this should be .end(). First, it's better to do COM_QUIT command and let server know that we wont to close connection. Second, I can add multiple queries to connection and release it without waiting for last command to complete. In that case .destroy() would kill all commands that hasn't been started
|
I think I slightly misunderstood what is in PR. You are not trying to actively open minConnections in advance, it's just number of connections that stay open despite being released for max idle time Why do you need extra array |
|
Also, maybe it's premature optimisation but I don't like that whole list of connections is scanned every second. We could just re-set timer to be ( now - last item in _freeConnections release time ) each time connection is aquired from freeConnections |
aikar
force-pushed
the
close-idle
branch
3 times, most recently
from
86ae7da to
e15daa4
Compare
|
@sidorares I believe i've resolved all concerns. got rid of the separate queue, added option to perform the automatic opens, clarified goals in the commits, and adjusted the timer to be 15 seconds instead of 1 second. Timer now only runs while we have extra connections, and only every 15 seconds unless your idle timeout is even smaller than that. |
This is to provide more context to error messages when a parameter is undefined.
aikar
force-pushed
the
close-idle
branch
2 times, most recently
from
44b2653 to
94f472f
Compare
This adds a new configuration for desired minimum connections, to complement the connection limit. If the autoOpenConnections is enabled (default true), the pool will immediately open the desired minimum number of connections. Any connection opened above the minimum, who sits unused for a time specified in idleTimeout, will be automatically closed. This allows you to set up a minimum expectation of standard load for connections to your database, but also allow you to configure a higher maximum to handle unexpected burst traffic, and be able to close them automatically when the burst subsides.
|
Closing while i still work out some quirks. I think i'm uncovering overall issues that had existed with closing connections, trying to work them out. |
|
Continued in #724 as silly github wont let me reopen. |
This is very similar to #321 but not the same. 321 is targeting cycling connections.
The goal if this PR is to allow users to set an upwards bounds on number of connections to a pool, but not keep every one of those connections open should it ever be hit.
This PR will let you specify a minimum number of desired connections to keep open, and any beyond that will automatically be closed after a period of inactivity.
Default configuration keeps existing behavior, by defaulting min = max, so no behavior change will apply to those who upgrade (semver minor)
This brings the pool inline with standard connection pool features, so you can set the min to a value that your application for the most part needs to keep open to avoid constantly opening new connections, but on the other hand allow temporary increases for out of the normal spikes in activity, but let them close out when no longer needed.
I know discussion of API was a concern with mysqljs, but I hope we can get this pulled and encourage mysqljs to also add support for this too.