-
-
Notifications
You must be signed in to change notification settings - Fork 941
[WIP] using db-pool library to create a pool of databases #5846
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
base: main
Are you sure you want to change the base?
Conversation
crates/db_schema/src/utils.rs
Outdated
| .await; | ||
|
|
||
| // TODO make compatible with ActualDbPool | ||
| db_pool.pull_immutable().await |
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 created this WIP PR to share the progress and the issue I'm stuck with currently. The crate I use operates with its own structure wrapping connection pools: code
And we have our own ActualDbPool. They are kinda same, but it's not obvious for me how to correctly convert one to another.
I had an idea to make ActualDbPool a enum with two possible values: RegularPool and ReusablePool, but stuck on trying to adapt stuff like LemmyContext, which also requires pool struct to be clone-able (and ReusablePool is not). And it seems a lot of changes to the main codebase for purely test changes.
Do you folks have any ideas how to manage that? Or should I stick to the initial plan without using this library?
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.
Our ActualDbPool is just a type alias for deadpool Pool<AsyncPgConnection>.
Their crate should be able to work with deadpool pools, but I'm not familiar with how to plug that into their crate... you'll have to ask them.
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.
Yeah, I see. I returned to this issue today after a week of a break. I'm in contact with the db-pool author and they're helping to understand a lot of moments and really willing to collaborate, so i think we'll make this work.
I'd like to clarify one moment: do we want build_db_pool_for_tests to return still ActualDbPool? db-pool has its own wrapper ReusableConnectionPool which works like a deadpool Pool, but a bit different and needs adaptation. And it might be easier to adapt tests for working with ReusableConnectionPool than converting ReusableConnectionPool to ActualDbPool
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.
The return type of build_db_pool_for_tests may be changed. Also, a DbPool variant may be added if needed.
crates/api/api_utils/src/context.rs
Outdated
| #[derive(Clone)] | ||
| pub struct LemmyContext { | ||
| pool: ActualDbPool, | ||
| pool: ContextPool, |
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 is the moment which currently blocks me, and i think it's better to consult with you again. LemmyContext structure must be cloneable, therefore all the fields, therefore the pool. But unfortunately, reusable pool from db-pool crate is not, and i don't have access to its fields to implement the trait here.
But before asking db-pool developer, i'd like to be sure we really need this pool cloning stuff, especially for the tests. Cloning the pool seems a bit strange to me, but i may miss something. I'm looking at the code now, but maybe you folks already have some insights on this
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.
Wrap it in Arc for now.
|
Update: I'm working on the topic; cannot devote much time for it, but it slowly going forward, and i keep the code in the branch up-to-date. I connected |
| db-pool = { git = "https://github.com/momentary-lapse/db-pool.git", branch = "edition2021-test-superuser", features = [ | ||
| "diesel-async-postgres", | ||
| "diesel-async-deadpool", | ||
| ] } |
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 should be a dev dependency. But dont worry about it for now, what matters is to get the parallel tests working.
|
What's this needing still? This is one of our oldest PRs. |
Basically, that's it. But other errors might appear. But yeah, apologies for horrendously delaying this PR. It required communication with another dev and adapting their library, but it's still too long |
|
Cool, no probs. I just know its a pain to maintain these things when we do a lot of refactor PRs, so its best to try to get the oldest ones merged first if possible. |
|
Update: managed to adapt the tests, but some of them fail. The reasons seem to be different, and I start with |
|
The initial error shown in CI is from These might be responsible for later failures. Also try to use the same db pool and same init code for all tests. By the way you can run specific tests locally with |
|
yes, i did run these locally scripts beforehand (also, |
|
fixed db views post tests, now only one of them fails: dunno why and what's the difference with others, will check later |
|
The markdown_links test is failing because line 141 has a second call to Other test failures are showing this second error below: By the way are tests not executed in parallel yet? Runtime seems the same as before. And please dont forget to remove docker-compose from test scripts so that it uses the local db like before. |
|
Okay, thanks, I hope to have time for the fixes today. Also noticed the performance didn't improve, probably need some other options—will check examples again and ask the author of the crate if won't find anything. And regarding docker-compose, I'll remove it in the very end. It's just handy to test the setup for me locally this way, while it's WIP |
|
actually decided to do it now. changed the db-pool in my fork to accept full db connection string and removed all the workaround for Unix socket connections |
Addresses: #4979