Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: pool adds gracefulExit to end gracefully #1810
base: master
Are you sure you want to change the base?
feat: pool adds gracefulExit to end gracefully #1810
Changes from 1 commit
5df4925
f39bce5
19705da
03ca7c2
737f4f8
b8ace42
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's intentional. I think graceful exit means finishing all queries called before end, so I make that happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
When will the acquiring condition will happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add documentation for the new queued argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This queued argument is a private argument, is it appropriate to add documentation to README.md?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.