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

fix(spanner): support transaction tags in partition DML #3473

Closed
wants to merge 4 commits into from

Conversation

rahul2393
Copy link
Contributor

@rahul2393 rahul2393 commented Nov 14, 2024

Fixes: #3347

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

@rahul2393 rahul2393 requested a review from a team as a code owner November 14, 2024 05:59
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/java-spanner API. labels Nov 14, 2024
@cloud-java-bot cloud-java-bot requested a review from a team as a code owner November 14, 2024 06:03
@@ -197,6 +197,10 @@ public static ReadQueryUpdateTransactionOption tag(String name) {
return new TagOption(name);
}

public static ReadQueryUpdateTransactionOption transactionTag(String name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this would bring in confusion to customers, mainly because transaction tags are supported in read-write transactions by using Options.tag. Customers might try using this Options.transactionTag for read-write and will not work.
https://cloud.google.com/spanner/docs/introspection/troubleshooting-with-tags#java_1

Should we rename this to be more specific to PDML or is this somehow restricted to be used only for pdml transaction?

@@ -194,22 +194,29 @@ ExecuteSqlRequest newTransactionRequestFrom(final Statement statement, final Opt
if (options.hasTag()) {
requestOptionsBuilder.setRequestTag(options.tag());
}
if (options.hasTransactionTag()) {
requestOptionsBuilder.setTransactionTag(options.transactionTag());
Copy link
Contributor

Choose a reason for hiding this comment

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

The official documentation states that transaction tags not supported for read-only transactions and are supported for read-write, but there’s no mention of partitioned DML transactions.
can you please verify if transaction tags are indeed supported for pdml txn?

@rahul2393
Copy link
Contributor Author

rahul2393 commented Nov 15, 2024

@rahul2393 rahul2393 closed this Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to provide transaction_tag with Partitioned DML update
4 participants