feat: allow Spanner and Datastore Transaction Manager in same project #1412
Conversation
Conditional for Spanner is based on missing beans of type SpannerTransactionManager instead of PlatformTransactionManager, this way it always gets created if Spanner libs are pulled in Conditional for Datastore is based on missing beans of type DatastoreTransactionManager instead of PlatformTransactionManager, this way it gets created if Datastore libs are pulled in It is user responsibility to designate the transaction's right transaction manager: ``` @transactional(transactionManager = "spannerTransactionManager") ``` ``` @transactional(transactionManager = "datastoreTransactionManager") ```
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Thanks for fixing this and sorry for letting this hang for so long!
Just one minor request: Do you mind also adding a note to the documentation that user needs to designate the transaction's right transaction manager if multiple transaction managers are brought in?
The corresponding sections in spanner documentation are here and here, datastore documentation are here and here. (We are keeping adoc and md copies of documentations temporarily)
|
On a separate note, I've verified this solution manually as we do not have a test setup with both Datastore and Spanner. @JoeWang1127 @meltsufin For future, what do you think about setting up an integration test with multiple data modules we support so we can capture bugs like this one? Or alternatively, there is an existing sample that we could add a test to. |
JoeWang1127
changed the title
|
I think that re-using the existing sample that already combines Spanner and Datastore makes sense. Just to clarify, this change doesn't require you to specify |
No change of behavior when only one started used. This is the case with existing IT tests with |
Done @zhumin8. Thanks for pointing out the exact location for the documentation. Let me know if you want any of the text changed. |
|
Hi @bijukunjummen, could you add a test in this sample to verify it's working when connecting Spanner and Datastore? |
Done @JoeWang1127 |
|
@bijukunjummen do you think we should change/add the integration tests in samples to verify the samples are working as expected? |
There was a problem hiding this comment.
Sample looks great! Simple and minimum additions.
Since you are at it, do you mind also adding a test similar to this existing one? It should basically do the same thing as your test, but the test runs in our CI so we are sure this functionality does not accidentally break in the future.
It could also be useful to mention this change in the sample readme, along the lines of “This sample also demonstrates usage of transactions with both modules.”
...-gcp-samples/spring-cloud-gcp-data-multi-sample/src/main/java/com/example/PersonService.java
Outdated
Show resolved
Hide resolved
...-gcp-samples/spring-cloud-gcp-data-multi-sample/src/main/java/com/example/TraderService.java
Outdated
Show resolved
Hide resolved
| @@ -854,6 +854,13 @@ This feature requires a bean of `DatastoreTransactionManager`, which is provided | |||
| If a method annotated with `@Transactional` calls another method also annotated, then both methods will work within the same transaction. | |||
| `performTransaction` cannot be used in `@Transactional` annotated methods because Cloud Datastore does not support transactions within transactions. | |||
|
|
|||
| Other Google Cloud database related libraries like spanner, firestore can introduce `PlatformTransactionManager` beans, and can interfere with Datastore Transaction Manager. To disambiguate, explicitly specify the name of the transaction manager bean for such `@Transactional` methods, for eg. | |||
There was a problem hiding this comment.
Just a wording suggestion. Please update the other instances as well if you like the rewording.
| Other Google Cloud database related libraries like spanner, firestore can introduce `PlatformTransactionManager` beans, and can interfere with Datastore Transaction Manager. To disambiguate, explicitly specify the name of the transaction manager bean for such `@Transactional` methods, for eg. | |
| Other Google Cloud database-related integrations like Spanner and Firestore can introduce `PlatformTransactionManager` beans, and can interfere with the Datastore Transaction Manager. To disambiguate, explicitly specify the name of the transaction manager bean for such `@Transactional` methods. Example: |
|
Kudos, SonarCloud Quality Gate passed!
|
zhumin8
changed the title
…#1412) Fixes #944 Conditional for Spanner is based on missing beans of type SpannerTransactionManager instead of PlatformTransactionManager, this way it always gets created if Spanner libs are pulled in Conditional for Datastore is based on missing beans of type DatastoreTransactionManager instead of PlatformTransactionManager, this way it gets created if Datastore libs are pulled in. It is the user's responsibility to designate the transaction's right transaction manager: ``` @transactional(transactionManager = "spannerTransactionManager") ``` ``` @transactional(transactionManager = "datastoreTransactionManager") ```
Fixes #944
Conditional for Spanner is based on missing beans of type SpannerTransactionManager instead of PlatformTransactionManager, this way it always gets created if Spanner libs are pulled in
Conditional for Datastore is based on missing beans of type DatastoreTransactionManager instead of PlatformTransactionManager, this way it gets created if Datastore libs are pulled in.
It is the user's responsibility to designate the transaction's right transaction manager: