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: Python client not backing off on retries when RetryInfo not present. #7303

Closed
crwilcox opened this issue Feb 8, 2019 · 7 comments
Closed
Assignees
Labels
api: spanner Issues related to the Spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@crwilcox
Copy link
Contributor

crwilcox commented Feb 8, 2019

When a session aborts, but doesn't have retry info python immediately retries. It seems to lack any sort of backoff here which may be incorrect. node attempts to.

https://github.com/googleapis/nodejs-spanner/blob/79f7855b3f6df59afe322dd578ed65f408a2fc2b/src/transaction.ts#L836

Python implementation currently returning 'None' if nothing is found:

Go: https://github.com/googleapis/google-cloud-go/blob/ab1512ccf757de1b7f7f62883d9f975dfc152cef/spanner/internal/backoff/backoff.go#L43

@crwilcox crwilcox added the api: spanner Issues related to the Spanner API. label Feb 8, 2019
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Feb 8, 2019
@tseaver tseaver added type: question Request for information or clarification. Not an issue. and removed triage me I really want to be triaged. labels Feb 11, 2019
@tseaver
Copy link
Contributor

tseaver commented Feb 11, 2019

IIRC, this was the behavior specified by the spanner team (retry immediately if no trailing headers specify a perior).

@snehashah16
Copy link

When a session aborts, but doesn't have retry info python immediately retries. It seems to lack any sort of backoff here which may be incorrect. node attempts to.

Do you mean a Transaction aborts ?

For java, we use a default value to backoff & then retry if retryDelay is not specified in the response:
https://github.com/googleapis/google-cloud-java/blob/master/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java#L2681-L2705

The current values are:
MIN_BACKOFF_MS = 1000;
MAX_BACKOFF_MS = 32000;

private static ExponentialBackOff newBackOff() {
return new ExponentialBackOff.Builder()
.setInitialIntervalMillis(MIN_BACKOFF_MS)
.setMaxIntervalMillis(MAX_BACKOFF_MS)
.setMaxElapsedTimeMillis(Integer.MAX_VALUE) // Prevent Backoff.STOP from getting returned.
.build();

Since I don't recollect any other context, I would default to the behavior of the Java Client lib.

@snehashah16
Copy link

Lets verify this behavior across each of our libraries and fix as needed.

@crwilcox crwilcox added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed type: question Request for information or clarification. Not an issue. labels Feb 15, 2019
@tseaver tseaver added the type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. label Feb 18, 2019
@tseaver
Copy link
Contributor

tseaver commented Feb 18, 2019

@crwilcox I presume that removing the 'type: question' label and adding a priority means you want this to have 'type: bug'.

@IlyaFaer
Copy link

I've been working on that issue, and got a question. Several times I saw lines like that, and I can't imagine, what is second addend for? First one (with pow) is ok, increments delay time, but the second one (with random) doesn't grow. So after 2 tries it become disproportionately small part of whole delay time and, as I see it, useless

@crwilcox
Copy link
Contributor Author

@IlyaFaer the random portion is to cause jitter in when we make requests and it shouldn't grow.

@tseaver
Copy link
Contributor

tseaver commented Jul 23, 2019

Via #8461.

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. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

5 participants