Useful options and performance improvements for Pool

[ifsnow]

This is an improved version of the previous PR(#1591). It's more readable and offers useful options and improved performance. Please work with this PR when you're ready to merge this feature later.

---

Useful options

It offers more useful options like Commons DBCP.

Option	Default	Description
testOnBorrow	true	Indicates whether the connection is validated before borrowed from the pool.
testOnBorrowInterval	20000	The number of milliseconds that indicates how often to validate if the connection is working since it was last used.
initialSize	0 (off)	The initial number of connections that are created when the pool is started.
maxIdle	10	The maximum number of connections that can remain idle in the pool.
minIdle	0 (off)	The minimum number of connections that can remain idle in the pool.
maxReuseCount	0 (off)	The maximum connection reuse count.
timeBetweenEvictionRunsMillis	0 (off)	The number of milliseconds to sleep between runs of examining idle connections.
minEvictableIdleTimeMillis	1800000	he minimum amount of time the connection may sit idle in the pool.
numTestsPerEvictionRun	3	The number of connections to examine during each run of the eviction timer.

See Readme.md

Improved performance

Performance is improved by over 80% without doing anything.

	Before	After
Executions per second	5,691	10,263

See benchmark code

More readable code

Focused on making the code easier to read and maintain for the following contributors, and tidy up wrong and unused logics.

---

I know that it's really hard to apply this right away because of people who use it in the wrong way such as accessing private underscore-prefix properties. Please consider it when major update is available later.

[dougwilson]

Wasn't there a similar pull request previously? I believe the biggest ask them was to please split it up into multiple pull requests, rather than one giant one, because it's basically unreviewable as it is, or at least I definitely won't be able to review it in the new few weeks while on vacation with the amount of code changed.

Also, looks like some CI failure.

[dougwilson]

Why rename this variable? Seems unrelated to the pull request. Please undo or explain how it fits into changing the pool.

[dougwilson]

Why rename this variable? Seems unrelated to the pull request. Please undo or explain how it fits into changing the pool.

[dougwilson]

Please undo the format change to this line.

[dougwilson]

So I did at least try to start reviewing, but quickly started to notice this is not backwards-compatible at all. I then noticed your note at the bottom of the pull request to consider it on the next major, so my bad! I deleted those comments :)

[ifsnow]

I think it's too difficult to contribute a little big change. It seems to be prudent because many people use this module. I fully understand and agree on your thought.

So, I guess it'd be better to create a new module that can operate independently without changing mysql module. I'll try on it.

You can close this PR or refer to this concept later on for your next improvement.

[AdriVanHoudt]

I would love to see this merged, the perf boost alone is 🔥
@ifsnow is there a way you think you can split up the changes in either multiple PRs or multiple commits? I think that would make it easier to review and merge

[dougwilson]

As far as I can tell, the biggest part of the perf boost is simply because with this PR, the connections are no longer always validated when you fetch them (the validation only happens every so often). Basically trading off some extra perf for a greater chance to end up with a bad connection from the pool. I'm not sure if that is a good trade off.

[dougwilson]

BTW, isn't there already an independently operating pool module? generic-pool or somethibg like that?

[sidorares]

sequelize and knex both use generic-pool

[dougwilson]

Ah, which based on help questions is a largr number of this module's users. I wonder: does it make sense for us to just use that module instead od trying to maintain out own pool implementation?

For example:

1. Should this module even provide a pool at all?
2. If it should, should it just be a precreated generic-pool instance?
3. If this module should have a pool buy not use generic-pool, what are the benefits?

[AdriVanHoudt]

oh if that is the tradeoff then that seems like a no go :P
providing a pool option in the lib is super useful and seems like a good and expected thing to have
the common module you are refering to is probably https://www.npmjs.com/package/generic-pool

[ifsnow]

@dougwilson I agree with your opinion, but I don't think it's efficient to validate the connection every time. That's because it's very unlikely that expected problems with a recently used connection will occur. So some guys say that turning off this option is recommended in many cases.

http://pubs.vmware.com/vfabric53/index.jsp?topic=/com.vmware.vfabric.tc-server.2.9/admin/manual-high-concurrency-jdbc.html
https://confluence.atlassian.com/adminjiraserver071/tuning-database-connections-802592189.html

I know about generic-pool. The reason I did not think about this is because I don't want to modify existing code.

If there is a difference of opinion, we don't need to discuss further.

[ifsnow]

mysql-pool-booster is for those who want to use experimental features of the pool.

I'm going to try various improvements about the pool in this project.