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

spanner: add a statement-based way to run read-write transaction. #2545

Merged
merged 10 commits into from
Jul 4, 2020

Conversation

hengfengli
Copy link
Contributor

@hengfengli hengfengli commented Jul 1, 2020

This adds the support to run the read-write transaction in the following way (no retries will be attempted and errors will be directly returned to the caller):

1. NewReadWriteStmtBasedTransaction
2. Execute statement 1
3. Execute statement 2
...
4. Commit or Rollback

Fixes #1971

@hengfengli hengfengli added the api: spanner Issues related to the Spanner API. label Jul 1, 2020
@hengfengli hengfengli requested a review from olavloite July 1, 2020 07:15
@hengfengli hengfengli requested a review from skuruppu as a code owner July 1, 2020 07:15
@hengfengli hengfengli self-assigned this Jul 1, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 1, 2020
Copy link
Contributor

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

A couple of general comments:

  1. Statement based transactions also need to be executed in a retry loop, only the user is now responsible for creating that loop one way or another. We should supply a clear example on how to use a statement based transaction, and emphasize the importance of a retry loop. Only checking the commit statement for an Aborted error is not enough, as any statement on the transaction can return with an Aborted error, and should cause the entire transaction to be retried.
  2. The Commit and Rollback methods should not be visible to users of the normal Client.ReadWriteTransaction method. Those methods would otherwise cause confusion, as they should not be called when using the normal Client.ReadWriteTransaction method.

spanner/client.go Outdated Show resolved Hide resolved
spanner/client.go Outdated Show resolved Hide resolved
spanner/client.go Outdated Show resolved Hide resolved
spanner/client.go Outdated Show resolved Hide resolved
spanner/client_test.go Outdated Show resolved Hide resolved
spanner/integration_test.go Outdated Show resolved Hide resolved
spanner/client.go Outdated Show resolved Hide resolved
spanner/transaction.go Outdated Show resolved Hide resolved
spanner/transaction.go Outdated Show resolved Hide resolved
spanner/transaction.go Outdated Show resolved Hide resolved
@hengfengli
Copy link
Contributor Author

@olavloite Please take another look. Thanks.

Copy link
Contributor

@olavloite olavloite 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 those changes. I'm very happy with the new ReadWriteTransactionStmtBased struct. Would you mind having a look to see if it would be doable to move the BeginTransaction call into that struct as well, and only have something like a NewReadWriteTransactionStmtBased method in client.go? That way, it won't stand out as much to new users who otherwise might start using it, not realizing that it is intended for libraries / ORMs etc.

spanner/client.go Outdated Show resolved Hide resolved
spanner/client_test.go Outdated Show resolved Hide resolved
spanner/client_test.go Outdated Show resolved Hide resolved
spanner/client_test.go Outdated Show resolved Hide resolved
spanner/integration_test.go Outdated Show resolved Hide resolved
spanner/integration_test.go Outdated Show resolved Hide resolved
spanner/transaction.go Outdated Show resolved Hide resolved
@hengfengli
Copy link
Contributor Author

hengfengli commented Jul 3, 2020

@olavloite Two things I want to mention in my latest change:

  1. Rename from extractRetryDelay to ExtractRetryDelay because we have to make it public to users.
  2. Add ExampleNewReadWriteStmtBasedTransaction in examples_test.go. This will be shown as an example in godoc.

Thanks for providing these nice suggestions. Please let me know if there are more issues. I am more than happy to fix them.

Copy link
Contributor

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

There are a couple of comments that now can be removed, but otherwise LGTM.

spanner/transaction.go Outdated Show resolved Hide resolved
spanner/transaction.go Outdated Show resolved Hide resolved
spanner/transaction.go Outdated Show resolved Hide resolved
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 Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner: allow users to disable internal retrying behaviour for Aborted or SessionNotFound errors
3 participants