gengapic: could deadlines be made configurable?
timburks opened this issue · 9 comments
gRPC deadlines are currently determined by timeout values specified in grpc service configurations (example).
As we've used the Go GAPICs, we've often wanted to adjust the timeouts used, e.g. this PR, which for us, involves changing the value in the service config and regenerating the GAPIC.
Reviewing the changes in the PR above, it seems that it might be easy to replace the hard-coded instances of the timeout value with a reference to a variable that is initialized with the configured value but overrideable through an API -- either by making the timeout variable public or by providing an API to get/set it.
We could patch the generated output, but it seems fairly harmless to add this to the core generator. Would you be open to a change like this?
tl;dr: Sure let's look at this, but I'd like to look at introducing a GAX option instead.
What you are referring to is modifying the default deadline for a method. The for an individual method call is configurable already, just set a deadline on the context.Context given to the RPC. What you want is to be able to change the default deadline for an RPC for the life of a Client instance i.e. client := NewClient(...); client.CallOptions.GetFoo = append(client.CallOptions.GetFoo, gax.WithTimeout(5 *time.second)), at least that's how I envision it as a gax call option, which is my preferred means of introducing such a "setting". This WithTimeout option would still defer to an existing deadline on the context.Context.
This seems pretty reasonable, and probably the easiest way to introduce a client setting that still respects the use of context.WithTimeout users my set.
That said, the config should still be the source of the default timeout for an RPC on all instances of the client, if unmodified, as it is today. I don't see anything wrong with your PR, it is the right thing to do, but is cumbersome if you only want to change the timeout for 1 RPC for 1 client instance in a specific application.
After discussing offline with @codyoss this seems like a good idea, and can also be pitched as code simplification/verbosity reduction if we move logic into a gax call option that is then implemented in gax.Invoke.
@noahdietz Would you want to file an issue for gax and move tracking to that issue?
Edit: I guess there will be work here to actually. But I still think we want a gax issue
@codyoss yep, good call, done! googleapis/gax-go#249
This sounds great! We create our clients in a centralized place, so setting the configuration there would be perfectly fine. So far, we haven't had any reason to adjust timeouts per method, just per endpoint - hosted versions of our service respond more slowly than local ones, etc. Our happy outcome would probably be to just have a global flag on our CLI that overrides the timeout that we use when making GAPIC calls, so any way that you prefer to enable that is great.
I'm going to start on implementing this today/tomorrow, hoping to have a generator release early next week. Rolling it out to existing GAPICs will need to be timed to follow the dependency update for gax-go in all client modules. @timburks you all, of course, can adopt it as soon as both are available.
Sorry for the insane delay on this. Going to release this imminently.
@timburks this was released in v0.37.0. The showcase integration testing was good, but if you've already started using this, please lmk how its gone so far, if there were any issues.
@noahdietz Thanks! We haven't tried this yet but will check it out. /cc @theganyo