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

Client-Side Retry Ignores 'maxAttempts' Configuration #545

Open
seaneganx opened this issue Feb 25, 2021 · 7 comments
Open

Client-Side Retry Ignores 'maxAttempts' Configuration #545

seaneganx opened this issue Feb 25, 2021 · 7 comments
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@seaneganx
Copy link

This should be reproducible on release v0.18.4.

The README mentions gRPC server config can be used for client-side retry configuration in accordance with AIP-4221. Most of the retry configuration is generated correctly, but the maxAttempts attribute is completely ignored.

I verified the limit is ignored by adding a log statement to the gax.APICall function passed into gax.Invoke to observe the retries continuing beyond the provided limit. The retries appeared to follow the policy backoff parameters, and the call option was otherwise generated correctly as shown below.

Policy:

"retryPolicy": {
  "initialBackoff": "0.250s",
  "maxBackoff": "2s",
  "maxAttempts": 8,
  "backoffMultiplier": 2,
  "retryableStatusCodes": [
    "UNKNOWN",
    "UNAVAILABLE"
  ]
}

Generated call option:

gax.WithRetry(func() gax.Retryer {
	return gax.OnCodes([]codes.Code{
		codes.Unknown,
		codes.Unavailable,
	}, gax.Backoff{
		Initial:    250 * time.Millisecond,
		Max:        2000 * time.Millisecond,
		Multiplier: 2.00,
	})
}),

Looking further into the gax package and specifically the Backoff struct, it doesn't look like there's any support for counting the number of attempts. The proposal here seems to indicate that an unavailable error should be returned after the max attempts are reached.

@noahdietz
Copy link
Collaborator

Yep, maxAttempts is not implemented in GAPICs (except for maybe Java but that was an external contribution and is not supported by the generator). Historically, we did not have a maxAttempts and during migration to this config format, we did not pick up as a feature. The threshold for number of attempts is the logical timeout of the method - the deadline set on the context in the GAPIC layer before being provided to gax-go.

GAPICs do not implement the gRPC retry proposal, the goal was to align on that starting by using the config, but it was never finished in gRPC and we've since had to go in a different direction with the addition of another GAPIC transport.

If you would like some functionality like maxAttempts, you can implement your own gax.Retryer and modify the CallOptions for your generated client (exposed, for example, here).

We are unlikely to implement this at the moment.

@noahdietz noahdietz added the type: question Request for information or clarification. Not an issue. label Feb 25, 2021
@noahdietz
Copy link
Collaborator

I'm going to close this for now. The feature is being investigated by the team more broadly and we may reopen this. Thanks!

@tam7t
Copy link

tam7t commented Jun 23, 2021

Could this be re-opened?

It appears that grpc-go does implement max retries but that this code generator explicitly disables the service configuration and uses the gax package which does has not been updated since 2019.

@noahdietz
Copy link
Collaborator

this code generator explicitly disables the service configuration

It just disables remote resolution of the grpc service config (docs). If one was provided via WithDefaultServiceConfig, theoretically it would work. However, I believe the gRPC wrapper Google clients use are currently forcing a specific config so IDK if that would interfere.

It appears that grpc-go does implement

Yes, but idk about Java, and I know C-core doesn't (it got deprioritized). So where possible, we'd prefer not to support things piecemeal language-wise, especially when there is no commitment to follow through on finishing it. My previous comment provides all of the context and a workaround.

I will reopen this, but we cannot prioritize/work on it anytime soon and I can't tell you if it ever will be worked on.

@noahdietz noahdietz reopened this Jun 23, 2021
@noahdietz noahdietz added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: question Request for information or clarification. Not an issue. labels Jun 23, 2021
@tam7t
Copy link

tam7t commented Jun 23, 2021

Thanks for re-opening

So where possible, we'd prefer not to support things piecemeal language-wise

Agreed! The context here is that I am configuring the service json for Secret Manager and trying to come up with better default retry parameters, and the differences between the supported language generators is confusing for a service team to understand what the actual client behavior will be.

@noahdietz
Copy link
Collaborator

Fair enough! Thanks for sharing, I understand the friction that comes with understanding implementations across languages.

@codyoss
Copy link
Member

codyoss commented Dec 22, 2022

In the future we should close this issue and create a tracking bug internally for a cross lang feature request as this does not seem to relate just to Go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

4 participants