-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add connection pool and connector svc #441
base: main
Are you sure you want to change the base?
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.
Excellent start, added my initial review for you. Based on how that progresses we might be ready for a final review already.
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.
Looks good. Can you also as part of this PR try to remove deadpool in the rama-cli fp code. Or what is your opinion on that? Seems to me like an excellent practical use case that also is a first good demonstration of this code in a real setting?
Not sure if this belongs in this PR, removing deadpool is one thing, but rama-cli also uses deadpool_postgres. These two combined actually do quite a lot of logic |
ead6f00
to
dc78122
Compare
rama-net/src/client/pool.rs
Outdated
} | ||
} | ||
|
||
pub fn with_wait_for_pool_timeout(mut self, timeout: Option<Duration>) -> Self { |
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 tend to call these kind of things maybe_with_x
and also provide a version with_x
without the Option wrpaper, and even a set_x
version for &mut self
(without the option wrapper).
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.
Mostly is looking good. I do agree that removing postgres-deadpool is probably out of scope for this PR.
Still, might be nice to add an example in /examples
that is also tested.
No description provided.