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

feat: allow Spanner and Datastore Transaction Manager in same project #1412

Merged
merged 7 commits into from
May 20, 2023

Conversation

bijukunjummen
Copy link
Contributor

@bijukunjummen bijukunjummen commented Dec 24, 2022

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")

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")
```
@sonarcloud
Copy link

sonarcloud bot commented Dec 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@zhumin8 zhumin8 left a comment

Choose a reason for hiding this comment

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

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)

@zhumin8
Copy link
Contributor

zhumin8 commented May 1, 2023

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 JoeWang1127 changed the title Changes conditional for Spanner and Datastore Transaction Manager feat: changes conditional for Spanner and Datastore Transaction Manager May 1, 2023
@meltsufin
Copy link
Member

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 transactionManager with the @Transactional, if only one of the staters is used?

@zhumin8
Copy link
Contributor

zhumin8 commented May 2, 2023

Just to clarify, this change doesn't require you to specify transactionManager with the @Transactional, if only one of the staters is used?

No change of behavior when only one started used. This is the case with existing IT tests with @Transactional in Spanner and Datastore and they both work as intended. (Integration tests are skipped for this PR because it's from a forked repo, but I've verified this behavior running from my machine)

@bijukunjummen
Copy link
Contributor Author

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)

Done @zhumin8. Thanks for pointing out the exact location for the documentation. Let me know if you want any of the text changed.

@JoeWang1127
Copy link
Contributor

Hi @bijukunjummen, could you add a test in this sample to verify it's working when connecting Spanner and Datastore?

@bijukunjummen
Copy link
Contributor Author

Hi @bijukunjummen, could you add a test in this sample to verify it's working when connecting Spanner and Datastore?

Done @JoeWang1127

@bijukunjummen bijukunjummen requested a review from zhumin8 May 9, 2023 02:42
@JoeWang1127
Copy link
Contributor

@bijukunjummen do you think we should change/add the integration tests in samples to verify the samples are working as expected?

Copy link
Contributor

@zhumin8 zhumin8 left a comment

Choose a reason for hiding this comment

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

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.”

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Just a wording suggestion. Please update the other instances as well if you like the rewording.

Suggested change
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:

Copy link
Contributor

@zhumin8 zhumin8 left a comment

Choose a reason for hiding this comment

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

LGTM.
I realized since integration test are not run for this pr, it's fair that adding sample test should not be part of it. I'll add it in a followup pr once this is in.

If you could address the comment on documentation wording, I am good to merge this in.

@sonarcloud
Copy link

sonarcloud bot commented May 19, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@zhumin8 zhumin8 changed the title feat: changes conditional for Spanner and Datastore Transaction Manager feat: allow Spanner and Datastore Transaction Manager in same project May 20, 2023
@zhumin8 zhumin8 merged commit f937b36 into GoogleCloudPlatform:main May 20, 2023
zhumin8 added a commit that referenced this pull request May 22, 2023
#1848)

This pr adds an integration test to `spring-cloud-gcp-data-multi-sample` for the scenario of both Datastore and Spanner Transaction manager working in the same app.
It is a followup on #1412
zhumin8 pushed a commit that referenced this pull request May 24, 2023
…#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")
```
zhumin8 added a commit that referenced this pull request May 24, 2023
#1848)

This pr adds an integration test to `spring-cloud-gcp-data-multi-sample` for the scenario of both Datastore and Spanner Transaction manager working in the same app.
It is a followup on #1412
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.

Transaction error using Spanner JDBC with JPA and Spring Cloud GCP Datastore in the same project
4 participants