New Pool options and refactoring. #1591

ifsnow · 2016-12-01T08:37:14Z

I hope Pool's features will get better like the others, so I added several options such as queueWaitTimeout, pingCheckInterval, startConnections, minSpareConnections, maxSpareConnections, spareCheckInterval
(See Readme.md)

These were what I aimed at.

Efficient code

Pool will work 10~50 times faster with new options.

	New (benchmark code)	Original (benchmark code)
Executions per second	1,292,685	22,415

Easy-to-read code

I've tried to make the code easier to read and maintain for the following contributors, and tidy up wrong and unused logics.

Backwards compatibility code

All defaults for new options are off. All tests are passed, so existing users can use it right now.
In my opinion, It's recommended that they use the new options as other pools do.

Here is my config below.

{
  connectionLimit : 50,
  queueWaitTimeout : 10000, // Same as acquireTimeout.
  pingCheckInterval : 10000, // The connection used in 10 seconds is reused without ping check.
  startConnections : 10, // 10 connections are created when the pool is started.
  minSpareConnections : 10, // 10 spare connections should be kept in the pool at all times.
  maxSpareConnections : 20, // No more than 20 spare connections.
  spareCheckInterval : 300000 // Check the spare connections every 5 minutes.
}

If you agree on my opinion, you can change the default value of new options. or, I think you could add a comment at Readme.md like this.

`pingCheckInterval`: ........ (Default: `0`, Recommendation: `10000`)

I hope this helps.
Please let me know if you have any concerns or advice.

…w user

dougwilson · 2016-12-05T02:08:19Z

Thanks a lot @ifsnow ! Is it possible to break this up into multiple PRs instead of one massive one? At the very lest to break it up into many different commits in the PR? It's a lot to really look through and understand the edge cases that will break people if this is applied without a major version bump.

dougwilson

Break apart so it can be reviewed easier.

dougwilson · 2016-12-05T02:10:37Z

test/unit/pool/test-destroy-connection.js

@@ -11,10 +11,10 @@ server.listen(common.fakeServerPort, function (err) {
  pool.getConnection(function (err, connection) {
    assert.ifError(err);

-    assert.strictEqual(connection, pool._allConnections[0]);
+    assert.strictEqual(connection, pool._connectionManager._allConnections._map[1]);


Even though this is an underscore-prefixed property, people really do use it, as I've seen throughout Stackoverflow and issues here. We can't change this without a major at this point. Hopefully it's possible to implement the idel timeout stuff without making this change.

It's a sad situation. I'll work to provide important old properties.

At this point in the module, probably every single property is important in some way. I would err on the site of just not changing them if you don't really need to.

I really understand. but, I think it's uncommon to use those properties such as Pool._acquiringConnections directly. It seems that it's not easy and efficient to work with all the existing ones.

I thought shortly.

pool.createPool({ ..., keepOldProperties : true });

If the user set this, 4 properties(_acquiringConnections, _allConnections, _freeConnections, _connectionQueue) are updated. Isn't it enough?

No, because if it's opt-in for backwards compatibility, then it's not backwards compatible, according to the semver spec. The old properties should be available by default without opt-in,

A quick Google search shows someone's code that is public (obviously there is probably way more non-public code) showing how people do use these properties: https://github.com/bobcui/potato/blob/e9f9def2dab26d738d45adfbf38743632a30278b/app/modules/mysql.js#L20

This PR does not make sense if keeping all properties is essential. I'm a little worried about what to do. Unfortunately, I think I'd better close this.

@ifsnow I think a lot of the pool optimizations are worth it, and certainly can make it into a major version of this module. The only real concern I have is that because this is just one giant PR, making good (but breaking) changes and making good but non-breaking changes, together, I have no option but to delay all the features in this PR until the next major. If you can simply break it up into the different pieces as multiple PRs, we can easily accept the easy ones now and discuss the breaking ones better and decide on the release of a new major version.

@dougwilson I really know how difficult it's to review many changes. I'm going to think about how I can break it up but, it's not easy. That's because PoolConnectionManager has all new functions. I wonder if it's possible to split this into multiple PRs easily.

dresende and others added 6 commits November 4, 2016 21:26

tests: adds mysql.format() coverage

Verified

This commit was signed with the committer’s verified signature.

ALiwoto ALi.w

GPG key ID: F453A002BB199A8F

Verified
Learn about vigilant mode

4015790

tests: adds mysql.Types coverage

dce49bd

tests: adds change user charset coverage

77a23c1

tests: adds another way of making a simple query with object params

c97737a

tests: updates change user test to cover calling without specifing ne…

7aa6da6

…w user

New Pool options and refactoring.

342cf60

This was referenced Dec 1, 2016

Add Pool options : maxWait, initialSize, minIdle, maxIdle #505

Closed

changeUser affects pool configuration!!! sidorares/node-mysql2#477

Open

dougwilson self-assigned this Dec 5, 2016

dougwilson requested changes Dec 5, 2016

View reviewed changes

dougwilson reviewed Dec 5, 2016

View reviewed changes

dougwilson force-pushed the master branch from 7aa6da6 to 88ad10c Compare January 14, 2017 02:43

ifsnow mentioned this pull request Mar 25, 2017

Do we have Minimum Connection Pool size support ? #1669

Open

dougwilson force-pushed the master branch 2 times, most recently from 638d79d to 88bade0 Compare June 29, 2017 18:13

ifsnow mentioned this pull request Jul 21, 2017

Useful options and performance improvements for Pool #1779

Closed

ifsnow closed this Jul 21, 2017

thirteen-eleven mentioned this pull request Sep 20, 2018

pool acquireTimeout - doesn't error when waiting #2105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Pool options and refactoring. #1591

New Pool options and refactoring. #1591

ifsnow commented Dec 1, 2016

dougwilson commented Dec 5, 2016

dougwilson left a comment

dougwilson Dec 5, 2016

ifsnow Dec 5, 2016

dougwilson Dec 5, 2016

ifsnow Dec 5, 2016

dougwilson Dec 5, 2016

ifsnow Dec 5, 2016

dougwilson Dec 5, 2016

ifsnow Dec 5, 2016

New Pool options and refactoring. #1591

New Pool options and refactoring. #1591

Conversation

ifsnow commented Dec 1, 2016

These were what I aimed at.

Efficient code

Easy-to-read code

Backwards compatibility code

dougwilson commented Dec 5, 2016

dougwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment