-
Notifications
You must be signed in to change notification settings - Fork 93
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Done.
The difference in logic is because PDML transactions cannot be committed or rolled back. This means much of the handling in This method currently does not take the config into account. I'll add that in shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
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.