Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

soundofspace
Copy link
Collaborator

No description provided.

Copy link
Member

@GlenDC GlenDC left a 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.

Copy link
Member

@GlenDC GlenDC left a 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?

@soundofspace
Copy link
Collaborator Author

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

@soundofspace soundofspace force-pushed the issue-107/implement-connection-pool branch from ead6f00 to dc78122 Compare March 6, 2025 08:23
}
}

pub fn with_wait_for_pool_timeout(mut self, timeout: Option<Duration>) -> Self {
Copy link
Member

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).

Copy link
Member

@GlenDC GlenDC left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants