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: add support for retrying aborted partitioned DML statements #66

Merged
merged 4 commits into from May 4, 2020

Conversation

larkee
Copy link
Contributor

@larkee larkee commented Apr 24, 2020

Partitioned DML transactions can be aborted by Cloud Spanner. These transactions were not automatically retried by the client library. This PR adds support for automatically retrying aborted partitioned DML statements.

@googlebot googlebot added the cla: yes label Apr 24, 2020
Copy link
Collaborator

@busunkim96 busunkim96 left a comment

Usage of api_core's retry looks good to me.

Just to check (I'm not at familiar with the Spanner library), this retries the whole transaction?

https://aip.dev/194#generally-non-retryable-codes

ABORTED: This code typically means that the request failed due to a sequencer check failure or transaction abort. These should not be retried for an individual request; they should be retried at a level higher (the entire transaction, for example).

@larkee
Copy link
Contributor Author

@larkee larkee commented Apr 28, 2020

Usage of api_core's retry looks good to me.

Just to check (I'm not at familiar with the Spanner library), this retries the whole transaction?

https://aip.dev/194#generally-non-retryable-codes

ABORTED: This code typically means that the request failed due to a sequencer check failure or transaction abort. These should not be retried for an individual request; they should be retried at a level higher (the entire transaction, for example).

Correct. It creates a new transaction on each retry attempt.

Copy link
Contributor

@skuruppu skuruppu left a comment

This looks reasonable but it would be good to add a description to this PR to explain why this came about.

Also, the retry logic in run_in_transaction looks quite different. It takes the retry config into account and does exponential backoff. Will this method of retrying take this config into account?

@larkee
Copy link
Contributor Author

@larkee larkee commented Apr 29, 2020

This looks reasonable but it would be good to add a description to this PR to explain why this came about.

Done.

Also, the retry logic in run_in_transaction looks quite different. It takes the retry config into account and does exponential backoff. Will this method of retrying take this config into account?

The difference in logic is because PDML transactions cannot be committed or rolled back. This means much of the handling in run_in_transaction does not apply here.

This method currently does not take the config into account. I'll add that in shortly.

@larkee larkee marked this pull request as ready for review Apr 30, 2020
@larkee larkee requested a review from skuruppu Apr 30, 2020
Copy link

@hengfengli hengfengli left a comment

LGTM.

@larkee larkee merged commit 8a3d700 into googleapis:master May 4, 2020
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants