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

storcon: switch to diesel-async and tokio-postgres #10280

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

Conversation

arpad-m
Copy link
Member

@arpad-m arpad-m commented Jan 3, 2025

Switches the storcon away from using diesel's synchronous APIs in favour of diesel-async.

Advantages:

We had to turn off usage of the connection pool for migrations, as diesel migrations don't support async APIs. Thus we still use spawn_blocking in that one place. But this is explicitly done in one of the diesel-async examples.

@arpad-m arpad-m requested a review from jcsp January 3, 2025 23:15
@arpad-m arpad-m requested a review from a team as a code owner January 3, 2025 23:15
Copy link

github-actions bot commented Jan 4, 2025

No tests were run or test report is not available

Test coverage report is not available

The comment gets automatically updated with the latest test results
a661209 at 2025-01-15T01:18:19.983Z :recycle:

@arpad-m
Copy link
Member Author

arpad-m commented Jan 4, 2025

Reconcile error: Cannot perform this operation while a transaction is open

not sure what the cause could be, I did a 1:1 translation, except that we don't start a transaction for running the migrations because of weiznich/diesel_async#115 . but that happens on an entirely separate connection from the reconciler where the issue occurs.

@conradludgate
Copy link
Contributor

I was curious so I tried with 1.85 with AsyncFn. It almost works, but there seems to be a limitation in proving Send bounds. I also had to manually implement the logic of TransactionManager.

@arpad-m
Copy link
Member Author

arpad-m commented Jan 8, 2025

I had Send issues as well, what worked is adding a Sync requirement. Not really beautiful, but it works :).

@arpad-m arpad-m force-pushed the arpad/diesel_async branch from 9775558 to 2f14a6e Compare January 13, 2025 02:35
@arpad-m
Copy link
Member Author

arpad-m commented Jan 13, 2025

Made it use r2d2 instead, but no avail, now it times out in the run function of TransactionBuilder when trying to figure out who the current leader is (basically hangs there).

@arpad-m arpad-m force-pushed the arpad/diesel_async branch from 2f14a6e to a661209 Compare January 14, 2025 23:57
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