-
Notifications
You must be signed in to change notification settings - Fork 191
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 retryable #359
base: main
Are you sure you want to change the base?
feat: add retryable #359
Conversation
@TimurSadykov we are getting requests for this behavior to be incorporated for certain spanner errors (see googleapis/google-cloud-php#5473). Can we go ahead and merge it? |
As we discussed at some point, we don't want to add retries, just return errors (exceptions), that indicate the error is retryable. If we can transition this into something like that - that would be great. |
@TimurSadykov Interesting, that sounds good. But if we indicate it's retryable, wouldn't we also want to allow those requests to be automatically retry? |
There was a long debate whether we should :) In short, the main reason not to retry is that every client can and should retry anyways. That is how few our languages historically do not have retries and do ok. However, for some of the languages client retries related to specifically auth are blocked by gRPC (described in go/auth-correct-retry). This could be perceived as retries layer is missing. Therefore we are fixing that first with retryable and gRPC fix. We can always revisit that and add retries later down the road. |
@TimurSadykov thank you for the thorough reply! So in the case mentioned by our customers (internal server errors returned by the Spanner service, NOT auth or grpc-specific errors), would it be safe or desirable to implement retry logic? For reference, Java has implemented this here: googleapis/java-spanner#2111 |
@bshaffer The overall retry approach is to rely on client side retries. Those are transparent and configurable. Java Spanner example sems to be inline with that. Those are client side retries. Not sure if default retry configuration applies to those retries though. But that would be minor, fixable deviation. Basically, client knows what to consider retryable from its own backend or transport. For Auth it is a bit different and clients don't know auth specifics. Therefore auth libraries encapsulates the logic of what is considered retriable and communicate to client via Retryable interface. |
Add automatic retriables for certain error codes. The feature would need to be implemented in
google/gax
to take effect. This is better for Backwards Compatibility and a better design in general. Other considerations:retryUnavailable
option, which defaults totrue
. Some clients will want to turn this off.