-
Notifications
You must be signed in to change notification settings - Fork 200
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
Honor pool connection timeout when executing queries directly in the pool #1338
base: 4.x
Are you sure you want to change the base?
Honor pool connection timeout when executing queries directly in the pool #1338
Conversation
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 change is good but will impact pooled clients for which the promise is to schedule commands without waiting for a connection to be free, so we should have this behaviour configurable and disabled when producing a pooled client. See PgBuilder#client()
vs PgBuilder#pool()
Thanks for the comment, will look into it and update the PR. |
…pool. Should fix eclipse-vertx#1232, as it now uses the timeout when acquiring the connection
- Found a pool test I could modify (but couldn't validate locally yet) - Changed the code to return the release the connection, which wasn't happening
…ile still ignoring timeout for legacy clients
a68b741
to
58dc3cd
Compare
@vietj finally got around to working on this one. I rebased with the 4.x branch and added the changes you requested. Let me know if you have any other comment. Thanks. |
One question, tho: I see that the |
@pablosaavedra-rappi yes you can push generated source code |
Looking at this again, I am not a fan of the option as is, I think instead command should have their own timeout to explicitely named the feature, and we should call it |
So @vietj you are suggesting we overload the method with an additional |
instead of having a boolean that says that the timeout applies to
everything, we have a timeout that applies to command execution scheduling.
…On Fri, May 17, 2024 at 4:39 PM pablosaavedra-rappi < ***@***.***> wrote:
So @vietj <https://github.com/vietj> you are suggesting we overload the
method with an additional scheduleTimeout parameter and the behavior
there? I can check how that'd look and update the PR.
—
Reply to this email directly, view it on GitHub
<#1338 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABXDCUNBDJ2FYM2AIJSMP3ZCYJAVAVCNFSM6AAAAAA2GR6U62VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJXG42TIOBSGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Should fix #1232, as it now uses the timeout when acquiring the connection
Motivation:
We've been bugged by #1232 for some time and the fix keeps getting pushed.