#3616 use retry settings when executing runWithRetries(Callable) #4683
Conversation
…able) The method SpannerImpl#runWithRetries(Callable) did not consider the retry settings that had been set on the SpannerOptions that created it. This could cause certain calls to wait indefinitely without ever breaking off with an exception.
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #4683 +/- ##
=========================================
Coverage 49.51% 49.51%
Complexity 22405 22405
=========================================
Files 2115 2115
Lines 211187 211187
Branches 24175 24175
=========================================
Hits 104570 104570
Misses 98384 98384
Partials 8233 8233Continue to review full report at Codecov.
|
|
CC @snehashah16 |
| } else { | ||
| backoffSleep(context, backOff); | ||
| } | ||
| if (options.getRetrySettings() != null) { |
There was a problem hiding this comment.
i would suspect the gax library being able to take care of these.
We should sync with gax|gapic team to help here and make this generic across all APIs and client libraries.
@hzyi-google - any thoughts on how to implement this?
There was a problem hiding this comment.
It does not seem this implementation is using any retry helpers provided by gax. what it's doing seems to be extracting retry configurations set in SpannerOptions and do manual retries here.
There was a problem hiding this comment.
Can we deprecate runWithRetries and actually use
https://github.com/googleapis/gax-java/blob/master/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java
There was a problem hiding this comment.
The Spanner library has its own implementation of runWithRetries(...), which also uses io.grpc.Context to allow the user to cancel calls or set timeouts. Re-writing this method, and thereby all the Spanner library, to use the standard helper methods from gax is probably a good idea. But as far as I can see, it will probably also have some side-effects to the current behavior.
Is that something we would want as part of solving this issue, or should that be a separate project?
There was a problem hiding this comment.
I agree with you that it will be ideal to use retry helpers in gax, and I also think it should be a separate project as well.
There was a problem hiding this comment.
@hzyi-google, should we close this PR and work on using the GAX retry helpers in another PR?
There was a problem hiding this comment.
@sduskis - we can have knut (or whoever) write up a design on migrating to gax, and its side-effects (or gaps in gax library) that Cloud Spanner may have a change in behavior.
I would start a new project and close this PR.
Also, there is a parallel thread for setting timeouts (and rpc cancellations) that should be included in this project (if possible) to make sure we dont drop any existing feature from the clients.
@crwilcox for timeouts & cancellations issue.
sduskis
added
do not merge
|
Sorry for the delay. This PR LGTM. But I think @snehashah16 should approve as well. |
|
@olavloite Can you provide the link/reference to this library RetryHelper#runWithRetries |
com.google.cloud.RetryHelper from google-cloud-core: https://github.com/googleapis/google-cloud-java/blob/master/google-cloud-clients/google-cloud-core/src/main/java/com/google/cloud/RetryHelper.java |
|
Replaced by #4940 |
7 participants
The method SpannerImpl#runWithRetries(Callable) did not consider the retry settings that had been set on the SpannerOptions that created it. This could cause certain calls to wait indefinitely without ever
breaking off with an exception.
This fix changes the SpannerImpl#runWithRetries(Callable) to consider the retry settings set on SpannerOptions.
I also considered changing the implementation into using RetryHelper#runWithRetries, but that API is (according to itself) still beta.
Fixes #3616