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

Transaction support for MongoDB with Panache #14002

Merged

Conversation

loicmathieu
Copy link
Contributor

@loicmathieu loicmathieu commented Dec 21, 2020

Fixes #11294

@loicmathieu
Copy link
Contributor Author

@FroMage I made this new prototype for Transaction support for MongoDB with Panache with some adjustements based on your feedbacks on #10097 (comment)

@loicmathieu we could perhaps skip supporting the reactive ones at first, and offer an equivalent to the transactional API that we have for HR+Panache. As for the initialisation of the client in the interceptor, I think you should be able to skip the interceptor entirely and delay the bit which registers your client to the TM until it's used. I'm pretty sure ORM does it this way: when you call an operation (not just obtain a client) we look at whether we have a current JTA transaction and if yes, we create a mongo transaction and register it to the TM (the same part you currently do in the interceptor).

So now, I don't use a CDI interceptor and I create the session when needed.

I need to re-implement everything as due to the changes for Kotlin support, my old branch cannot be rebased, as it'll take some time I want to be sure that this is the good direction before going on so I just implement one method to show you how it will be used.

If you validate the design, I'll report all the changes from my old branch.

With this design, I'll still need to find a way to pass the transaction options from the calling method (to be discussed later).

Base automatically changed from master to main March 12, 2021 15:55
@FroMage
Copy link
Member

FroMage commented Mar 12, 2021

Sorry it took such a long time. This looks just fine to me :)

@loicmathieu
Copy link
Contributor Author

OK great !
I'll finish the implementation and we can discuss it more

@loicmathieu loicmathieu force-pushed the mongodb-panache/narayana-transaction-3 branch from b92b37a to a46b900 Compare March 15, 2021 18:25
@loicmathieu loicmathieu force-pushed the mongodb-panache/narayana-transaction-3 branch from a46b900 to b8c30e2 Compare March 16, 2021 13:21
@loicmathieu loicmathieu marked this pull request as ready for review March 16, 2021 13:29
@loicmathieu loicmathieu force-pushed the mongodb-panache/narayana-transaction-3 branch 2 times, most recently from 87a72f9 to 37e131b Compare March 19, 2021 10:04
@loicmathieu
Copy link
Contributor Author

loicmathieu commented Mar 19, 2021

@gytis I implemented transaction support for MongoDB with Panache and it crashed the integration test of rest data support for MongoDB with Panache.

Apparently, you add the @Transactional annotation even for MongoDB, perviously it has no effect, now it uses a transaction so if the user didn't use a replicat set (and a version that support transaction) it failed.

I think the best would be to disabled transaction for MongoDB via Rest Data Panache, I'm not sure how to do this.

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.

Just had the one question about that exception but otherwise looks nice.

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.

LGTM, but you may want to add the following methods?

public class Panache {

     /**
     * Returns the current {@link TransactionManager}
     *
     * @return the current {@link TransactionManager}
     */
    public static TransactionManager getTransactionManager() {
        return AbstractJpaOperations.getTransactionManager();
    }

   /**
     * Marks the current transaction as "rollback-only", which means that it will not be
     * committed: it will be rolled back at the end of this transaction lifecycle.
     */
    public static void setRollbackOnly() {
        AbstractJpaOperations.setRollbackOnly();
    }

}

I'm not sure if TransactionManager is visible in Mongo, now that you depend on narayana.

@loicmathieu
Copy link
Contributor Author

@FroMage there is no global Panache helper class on MongoDB with Panache and I'm reluctant to add one to only provide a shorter path for this. One can simply inject the TransactionManager if he want to do it.

@FroMage
Copy link
Member

FroMage commented Mar 22, 2021

@FroMage there is no global Panache helper class on MongoDB with Panache and I'm reluctant to add one to only provide a shorter path for this. One can simply inject the TransactionManager if he want to do it.

Oh, I didn't know.

@loicmathieu
Copy link
Contributor Author

loicmathieu commented Mar 23, 2021

@gytis can you have a look at the following commit: 20edd61

I manage to make the REST data impl working for both Hibernate and Mongo by allowing to use a different update operator to avoid having a transaction for MongoDB.

@FroMage if you know this part, maybe you can also have a look at it.

@loicmathieu loicmathieu force-pushed the mongodb-panache/narayana-transaction-3 branch from 3fe2a97 to 3b1122a Compare March 23, 2021 12:18
@loicmathieu loicmathieu force-pushed the mongodb-panache/narayana-transaction-3 branch from 3b1122a to 20edd61 Compare March 24, 2021 10:11
@quarkus-bot quarkus-bot bot added the area/spring Issues relating to the Spring integration label Mar 24, 2021
@gytis
Copy link

gytis commented Mar 25, 2021

@loicmathieu sorry for a delay. I was on PTO since the middle of the last week.
I wasn't aware of this MongoDB transaction effort, I would have taken it into account when adding the transactional executor. I only added it a couple of weeks ago. In any case, I think your fix looks good.

@loicmathieu
Copy link
Contributor Author

@gytis thanks for looking at it

@loicmathieu sorry for a delay. I was on PTO since the middle of the last week.

No problem for the delay, it gives me the opportunity to have a look at the impl of rest data and it's an interesting piece of code ;)

@loicmathieu loicmathieu added this to the 1.14 - main milestone Mar 25, 2021
@loicmathieu loicmathieu merged commit dcb2415 into quarkusio:main Mar 25, 2021
@loicmathieu loicmathieu deleted the mongodb-panache/narayana-transaction-3 branch March 25, 2021 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants