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

Provide programmatic transaction for MongoDB reactive with Panache #32794

Merged
merged 1 commit into from Jun 8, 2023

Conversation

loicmathieu
Copy link
Contributor

Fixes #32156

@loicmathieu
Copy link
Contributor Author

@FroMage this is a PoC for MongoDB with Panache reactive transaction based on what you did for HIbernate with Panache. Before I implement the transaction support for all operations can you check the way I handle transactions is the new Panache class ?

@quarkus-bot

This comment has been minimized.

Copy link
Member

@evanchooly evanchooly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look fine. The failing tests are odd, though.

@loicmathieu
Copy link
Contributor Author

@evanchooly thanks, I'll have a look at the failing test soon.

@quarkus-bot

This comment has been minimized.

@loicmathieu
Copy link
Contributor Author

@evanchooly I fixed the tests, this was due to the fact that a Vert.x context was required for reactive transactions but it should not be required out of the context of a transaction so I updated the code accordingly (a Vert.x context is only required to in a transaction). I also added a small section on the documentation, can you have a look please?

@quarkus-bot

This comment has been minimized.

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks alright to me, minus the context removal question.

What's the point about transactions in mongo again? IIRC they require replicas, right?

Do you think you should test transactions to run multiple things in isolation? I don't think the tests do that.

Also, do you intend to also provide the equivalent of @WithTransaction?

Quarkus Documentation automation moved this from To do to Review pending Jun 5, 2023
@loicmathieu
Copy link
Contributor Author

@FroMage

What's the point about transactions in mongo again? IIRC they require replicas, right?

They are lightweight transactions and require a replica set, but you can configure a replicaset of one node (our devservices is configured this way). Basically, you open a session and pass it to all calls to the MongoDB driver, and at the end you commit or rollback. So the only trick is to store the session somewhere, in the Vert.x context here.

Do you think you should test transactions to run multiple things in isolation? I don't think the tests do that.

I agree test can be improved, but the transaction for the sync client has the same test. I don't remember in details the isolation model of MongoDB transactions, I can open a followup issue to improve our test coverage for it.

Also, do you intend to also provide the equivalent of @WithTransaction?

Maybe someday depending on my todo list ;) I need to check how it was done on HIbernate reactive and take inspiration.

@evanchooly
Copy link
Member

the docs changes look fine to me as well.

@FroMage
Copy link
Member

FroMage commented Jun 5, 2023

They are lightweight transactions and require a replica set, but you can configure a replicaset of one node (our devservices is configured this way). Basically, you open a session and pass it to all calls to the MongoDB driver, and at the end you commit or rollback. So the only trick is to store the session somewhere, in the Vert.x context here.

OK thanks.

Do you think you should test transactions to run multiple things in isolation? I don't think the tests do that.

I agree test can be improved, but the transaction for the sync client has the same test. I don't remember in details the isolation model of MongoDB transactions, I can open a followup issue to improve our test coverage for it.

Well, at least you're testing that we can roll stuff back :) But yeah, isolation is probably worth testing.

Maybe someday depending on my todo list ;) I need to check how it was done on HIbernate reactive and take inspiration.

You'd better check that this isn't what the original user wanted in #32156, because otherwise you're both going to be disappointed ;)

@loicmathieu
Copy link
Contributor Author

@FroMage if it's OK with the context local removal I want to proceed with the PR as is, I'll create followup issue for more test on MongoDB transaction support (both sync and reactive) and a followup to support annotation based transactions.

@quarkus-bot

This comment has been minimized.

@github-actions
Copy link

github-actions bot commented Jun 6, 2023

🙈 The PR is closed and the preview is expired.

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, all good, thanks!

Quarkus Documentation automation moved this from Review pending to Reviewer approved Jun 8, 2023
@FroMage FroMage merged commit 51cb966 into quarkusio:main Jun 8, 2023
23 checks passed
Quarkus Documentation automation moved this from Reviewer approved to Done Jun 8, 2023
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jun 8, 2023
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone Jun 8, 2023
@gsmet gsmet changed the title Provide programmative transaction for MongoDB reactive with Panache Provide programmatic transaction for MongoDB reactive with Panache Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Transactions support for reactive entities and repositories in mongodb panache
3 participants