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 reactive transaction support SPI #22646

Merged
merged 1 commit into from May 2, 2019

Conversation

mp911de
Copy link
Member

@mp911de mp911de commented Mar 23, 2019

This commit adds SPI interfaces to support reactive transactions through spring-tx with optional dependencies to Project Reactor and supportive implementations for TransactionalOperator and AbstractReactiveTransactionManager.

See #22590 for more background. The implementation is mainly inspired by spring-tx leveraging Reactor's Subscriber Context.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 23, 2019
@sbrannen sbrannen added in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement labels Mar 23, 2019
@jhoeller
Copy link
Contributor

@mp911de Thanks for turning it into a PR, looks like a fine start for integration with @Transactional!

Just one immediate thing: Could you please move the transaction.reactive.support contents to transaction.reactive itself since the latter seems to be empty at this point? The reactive subpackage would then effectively be the support-like package for reactive transactions anyway... Would be great to sort this out within the PR still.

@jhoeller jhoeller self-assigned this Mar 23, 2019
@jhoeller jhoeller removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 23, 2019
@jhoeller jhoeller added this to the 5.2 M1 milestone Mar 23, 2019
This commit adds SPI interfaces to support reactive transactions
through spring-tx with optional dependencies to Project Reactor and
supportive implementations for TransactionalOperator and
AbstractReactiveTransactionManager.
@mp911de
Copy link
Member Author

mp911de commented Mar 23, 2019

Resolved reactive.support into reactive now.

@jhoeller
Copy link
Contributor

Running out of time for 5.2 M1, rather expectedly... Let's make this a 5.2 M2 topic, first thing once M1 is out. I hope that's also early enough for Spring Data's purposes.

@jhoeller jhoeller modified the milestones: 5.2 M1, 5.2 M2 Mar 27, 2019
@mp911de
Copy link
Member Author

mp911de commented Mar 27, 2019

That should work for us.

@jhoeller jhoeller merged commit beea83b into spring-projects:master May 2, 2019
jhoeller added a commit that referenced this pull request May 2, 2019
Includes general revision of reactive transaction sources.

See gh-22646
@jhoeller
Copy link
Contributor

jhoeller commented May 2, 2019

This is in master now, including a first pass with a revision of mine that removes all legacy configuration options from AbstractReactiveTransactionManager plus a few further things. The intent is to keep the reactive transaction abstraction rather streamlined, not burdening it with JTA/JPA-oriented options that AbstractPlatformTransactionManager historically had to deal with. Let me know if anything important is missing now for the Mongo/R2DBC implementations; we can of course consider reintroducing relevant options through template methods or the like (ideally not through boolean setters again).

A second pass is coming in just a bit, renaming some of the types in transaction.reactive through dropping the "Reactive" prefix: TransactionSynchronizationManager, TransactionCallback etc read much nicer in the corresponding API declarations and are locally prefixed by "reactive" through their containing package anyway. Also, they don't immediately conflict with the same-named types in transaction.support, whereas we have to use specific prefixes for new types in the transaction base package (e.g. ReactiveTransactionStatus vs TransactionStatus). We applied the same reasoning to our reactive HTTP abstractions, using same-named types in quite a few cases there as well.

@mp911de Please wait for that renaming commit before integrating the Spring Data bits with it.

@jhoeller
Copy link
Contributor

jhoeller commented May 2, 2019

I also consider renaming ReactiveTransactionStatus to simply ReactiveTransaction, making it extend a common TransactionExecution interface that has the common methods with TransactionStatus... and shortening its implementation name to GenericReactiveTransaction, making the template methods in AbstractReactiveTransactionManager nicer.

@jhoeller
Copy link
Contributor

jhoeller commented May 2, 2019

@mp911de Are you ok with the shortened type names suggested above? It's ready for committing from my side.

@jhoeller
Copy link
Contributor

jhoeller commented May 2, 2019

Being at it, I also intend to drop ReactiveResourceHolderSynchronization, making the transaction.reactive package completely decoupled from transaction.support. I wouldn't recommend building any reactive resource support with the transaction.support.ResourceHolder type which is really quite oriented towards thread binding (isVoid etc don't make much sense otherwise); if needed, we can introduce a dedicated reactive resource holder variant.

@mp911de
Copy link
Member Author

mp911de commented May 2, 2019

Thanks a lot for merging this one. I'm fine with shorter names. I'm always torn having multiple types with the same name.

Right now, when using multiple transactional resources, it's sufficient to keep a single transaction manager and additional resources can join by registering ReactiveResourceHolderSynchronization.

By removing ReactiveResourceHolderSynchronization, we no longer have this possibility and only a single transactional resource can be used in a reactive transaction.

@jhoeller
Copy link
Contributor

jhoeller commented May 2, 2019

So who would use ReactiveResourceHolderSynchronization - is it primarily integration code of ours (which could include a custom variant of that class for its purposes) or general user code? If we keep shipping such a class out of the box, we should at least decouple it from transaction.support.ResourceHolder...

@jhoeller
Copy link
Contributor

jhoeller commented May 2, 2019

Hmm it seems that ReactiveResourceHolderSynchronization is just generically typed to the ResourceHolder class anyway... We could simply remove that generic bound and decouple it from ResourceHolder that way.

jhoeller added a commit that referenced this pull request May 2, 2019
Introduces TransactionExecution base interface for TransactionStatus as well as ReactiveTransaction. Renames getTransaction method to getReactiveTransaction, allowing for combined implementations of PlatformTransactionManager and ReactiveTransactionManager.

See gh-22646
@mp911de
Copy link
Member Author

mp911de commented May 3, 2019

It would make sense to keep static TransactionalOperator create(ReactiveTransactionManager transactionManager) as an additional factory method, without requiring DefaultTransactionDefinition.

@jhoeller
Copy link
Contributor

jhoeller commented May 3, 2019

The main reason for dropping it was the use of DefaultTransactionDefinition which lives in transaction.support and introduces an unwanted dependency that the transaction.reactive package overall doesn't have anymore. If we find this important enough to have, we could reintroduce it and use some local dummy TransactionDefinition instead; after all, we just need an instance which returns default values, not a modifiable instance like DefaultTransactionDefinition.

Thinking about it, we could even have a static defaults accessor on the TransactionDefinition interface itself, returning an unmodifiable instance... That would be convenient for users as well, not having to use DefaultTransactionDefinition for simple purposes.

@jhoeller
Copy link
Contributor

jhoeller commented May 3, 2019

That seems to work nicely with default methods on the TransactionDefinition interface itself. I'll reintroduce static TransactionalOperator create(ReactiveTransactionManager transactionManager) with an implementation along those lines.

@mp911de
Copy link
Member Author

mp911de commented May 3, 2019

I found two bugs during the integration in R2DBC and MongoDB (doCleanupAfterCompletion does not get subscribed to and supposedly TransactionalOperator sends a cancellation signal after a few emitted elements). I'll provide either today or Monday a PR to fix these.

jhoeller added a commit that referenced this pull request May 3, 2019
Such a static unmodifiable TransactionDefinition with defaults can be used for getTransaction(null) calls, now also possible for getReactiveTransaction. Furthermore, it can be used for a simple TransactionalOperator.create(ReactiveTransactionManager) method without an internal dependency on the transaction.support package.

See gh-22646
@jhoeller
Copy link
Contributor

jhoeller commented May 3, 2019

Alright, good to catch those bugs early enough still...

The TransactionalOperator revision with the overloaded create method is in master now. I really like the new TransactionDefinition.withDefaults() method, reads nicely in several of our own code spots...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants