-
Notifications
You must be signed in to change notification settings - Fork 781
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
gRPC client retries #1187
gRPC client retries #1187
Conversation
200ee48
to
57c381e
Compare
3d8fcc7
to
2152cd6
Compare
e549c80
to
349c91f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lazy config model doesn't seem like a good fit here. It results in a lot of per-request validation and parsing (sometimes repeatedly per request) that could have been done once up front when creating the channel.
f2db3e6
to
5df6bf2
Compare
c296267
to
6c53dfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good though I wouldn't be surprised if there are some synchronization corner cases that we missed. Given that the features added are marked with experimental status, I'm okay with merging the changes and fix issues that come up.
Let's wait for @jtattermusch to take a look as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, but overall LGTM. Thanks for implementing this!
🍰 🥳 🎈 🎉 |
Fixes #1189
This PR adds support for gRPC retries. When retries are enabled, a wrapping call (either
RetryCall
orHedgingCall
) is created that will manage making HTTP requests to the server. The wrapping call will createGrpcCall
instances that will perform the HTTP request to the server.GrpcCall
is mostly unchanged.RetryCall
andHedgingCall
(and their base type) handle retry logic, message buffering, throttling, etc. Once a gRPC call is commited then reposibility is handled back to it for returning a response.ServiceConfig
type for client configuration. Currently it is only ever manually created by the client, but in the future deserializing from JSON/Protobuf will be supported for populating it by a naming resolver.GrpcCall
without performance impact of features required by retries.Other bugs fixed during this PR:
Fix - An error from the server to the client will now correctly update the status of in-progress server streaming and bidirectional streaming calls. This matches Grpc.Core behavior.Fix response stream errors not updating call status #1198Change - A completed client streaming and bidirectional streaming call in the client will throwChange some RequestStream.WriteAsync errors to match Grpc.Core #1199RpcException
fromRequestStream.WriteAsync
. This matches Grpc.Core behavior.Change - Don't log cancellation exception from HttpClient on deadline exceeded in client.Fix deadline timer rescheduling because of timer precision #1201TODO
ResponseStream.MoveNext
cancellationWork in progress on https://github.com/grpc/proposal/blob/master/A6-client-retries.md