Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upfeat: pool adds gracefulExit to end gracefully #1810
Conversation
|
A couple comments and looks like the coverage check failed because there are uncovered cose paths introduced in this change. If you need help coming up with a test at all, let me know and I can lend a hand :) |
| If `gracefulExit` is set to `true`, the connections end _gracefully_, so all | ||
| -pending queries will still complete and the time to end the pool will vary. | ||
|
|
||
| The default `gracefulExit` is `false`, the following behavior will take effect. |
dougwilson
Aug 30, 2017
Member
I think the exact details of how the graceful end works here would be useful, as I remember that was the initial confusion here. As far as I can tell, this implementation will still let checked out connections queue new queries if there is at least one connection in the middle of being established, since the QUIT commands are not queued on all the connections until there is no connection in the aquiring state, is that intentional?
I think the exact details of how the graceful end works here would be useful, as I remember that was the initial confusion here. As far as I can tell, this implementation will still let checked out connections queue new queries if there is at least one connection in the middle of being established, since the QUIT commands are not queued on all the connections until there is no connection in the aquiring state, is that intentional?
HQidea
Sep 9, 2017
Author
Yes, it's intentional. I think graceful exit means finishing all queries called before end, so I make that happen.
Yes, it's intentional. I think graceful exit means finishing all queries called before end, so I make that happen.
dougwilson
Sep 10, 2017
Member
Right, but my comment was that your code is still sometimes allowing queries to be called after end. Is the call to end a hard stop, or should new queries still be allowed to get scheduled? The other issue is that under the acquiring condition, it will sometimes then stop the checked out connections from continuing to query and sometimes not.
Right, but my comment was that your code is still sometimes allowing queries to be called after end. Is the call to end a hard stop, or should new queries still be allowed to get scheduled? The other issue is that under the acquiring condition, it will sometimes then stop the checked out connections from continuing to query and sometimes not.
HQidea
Jun 25, 2019
Author
Right, but my comment was that your code is still sometimes allowing queries to be called after end. Is the call to end a hard stop, or should new queries still be allowed to get scheduled?
Only queries have been called before end would be allowed to execute, new queries after end would not be allowed no matter whether there is any checked out connection in the pool
The other issue is that under the acquiring condition, it will sometimes then stop the checked out connections from continuing to query and sometimes not.
When will the acquiring condition will happen?
Right, but my comment was that your code is still sometimes allowing queries to be called after end. Is the call to end a hard stop, or should new queries still be allowed to get scheduled?
Only queries have been called before end would be allowed to execute, new queries after end would not be allowed no matter whether there is any checked out connection in the pool
The other issue is that under the acquiring condition, it will sometimes then stop the checked out connections from continuing to query and sometimes not.
When will the acquiring condition will happen?
| } | ||
|
|
||
| Pool.prototype.getConnection = function (cb) { | ||
| Pool.prototype.getConnection = function (cb, queued) { |
dougwilson
Aug 30, 2017
Member
Please add documentation for the new queued argument.
Please add documentation for the new queued argument.
HQidea
Sep 9, 2017
Author
This queued argument is a private argument, is it appropriate to add documentation to README.md?
This queued argument is a private argument, is it appropriate to add documentation to README.md?
dougwilson
Sep 10, 2017
Member
There is no such thing as a private argument -- the user can simply pass in the value. Just not documenting it doesn't make it private. If the argument is there, it needs to be documented.
There is no such thing as a private argument -- the user can simply pass in the value. Just not documenting it doesn't make it private. If the argument is there, it needs to be documented.
|
Can you add at least one test that tests the issue you were having: in a pool of size |
|
Sorry to fix late, and a test case as you requested was added. |
dougwilson
force-pushed the
mysqljs:master
branch
2 times, most recently
from
946727b
to
37fbbdd
|
I'd love to see this feature. Any progress on that? |
|
@dougwilson I updated the Readme.md because of conflicts. So is there any chance to merge this pr? |
|
Hi @HQidea I really apologize on this one. I looked over the state of the PR when @Sprax2013 commented trying to remember why it wasn't merged. I got to work and found the reason in some notes I had: I meant to update the PR to pass options through the PoolCluster, which wraps around Pool so they could utilize the new end options added here. I completely lost track of that, and I'm sorry for that. |
|
Hi @dougwilson I added the option to PoolCluster.end. And should I add the option to PoolCluster.remove too? |
|
unsubscribe this account
On Friday, June 28, 2019, 4:55:06 AM CDT, HQidea <[email protected]> wrote:
Hi @dougwilson I added the option to PoolCluster.end. And should I add the option to PoolCluster.remove too?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
As we discussed at #1803, this pull request adds an option to pool's creation,
If
gracefulExitistrue, everypool.getConnectionorpool.querycalled beforepool.endwill success. Iffalse, only commands / queries already in progress will complete, others will throw an error.