-
Notifications
You must be signed in to change notification settings - Fork 318
Support grpc-retry-pushback-ms #664
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
Open
DylanRussell
wants to merge
15
commits into
open-telemetry:main
Choose a base branch
from
DylanRussell:patch-1
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+22
−22
Open
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
e8f3005
Propose updated language around gRPC error retry handling
DylanRussell 2dc17a2
Update specification.md
DylanRussell 11975a4
Update specification.md
DylanRussell 908de69
Update docs/specification.md
DylanRussell 62aaad1
Update docs/specification.md
DylanRussell 7e3f438
Update specification.md
DylanRussell c8728fe
Merge remote-tracking branch 'origin' into patch-1
DylanRussell 695f349
Update spec to say the client should respect only RetryInfo or grpc-r…
DylanRussell 60b16d8
Commit reviewers suggestions
DylanRussell f982a8d
Merge branch 'main' into patch-1
DylanRussell e24a9d6
Update docs/specification.md
pellared 61fbef5
Fix lint issues
DylanRussell 60676b3
Merge branch 'patch-1' of github.com:DylanRussell/opentelemetry-proto…
DylanRussell d6d9036
Fix lint issues
DylanRussell 8ccfd8c
Merge branch 'main' into patch-1
pellared File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -240,7 +240,6 @@ Here is a sample Go code to illustrate: | |
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
|
|
||
| return st.Err() | ||
| ``` | ||
|
|
||
|
|
@@ -314,8 +313,13 @@ error with code [Unavailable](https://godoc.org/google.golang.org/grpc/codes) | |
| and MAY supply additional | ||
| [details via status](https://godoc.org/google.golang.org/grpc/status#Status.WithDetails) | ||
| using | ||
| [RetryInfo](https://github.com/googleapis/googleapis/blob/6a8c7914d1b79bd832b5157a09a9332e8cbd16d4/google/rpc/error_details.proto#L40). | ||
| Here is a snippet of sample Go code to illustrate: | ||
| [RetryInfo](https://github.com/googleapis/googleapis/blob/6a8c7914d1b79bd832b5157a09a9332e8cbd16d4/google/rpc/error_details.proto#L40) | ||
| or via Trailer [with the gRPC metadata key `grpc-retry-pushback-ms`](https://github.com/grpc/proposal/blob/master/A6-client-retries.md#pushback). | ||
|
|
||
| Some clients may only respect one of these pushback mechanisms, so it's recommended for the server to implement both if it's | ||
| critical that it be respected. | ||
|
DylanRussell marked this conversation as resolved.
Outdated
|
||
|
|
||
| Here is a snippet of sample Go code to illustrate using `RetryInfo`: | ||
|
|
||
| ```go | ||
| // Do this on the server side. | ||
|
|
@@ -340,29 +344,23 @@ Here is a snippet of sample Go code to illustrate: | |
| } | ||
| } | ||
| ``` | ||
| Here is a snippet of sample Python code to illustrate using `grpc-retry-pushback-ms`: | ||
|
DylanRussell marked this conversation as resolved.
Outdated
|
||
|
|
||
| When the client receives this signal, it SHOULD follow the recommendations | ||
| outlined in documentation for | ||
| [RetryInfo](https://github.com/googleapis/googleapis/blob/6a8c7914d1b79bd832b5157a09a9332e8cbd16d4/google/rpc/error_details.proto#L40): | ||
|
|
||
| ``` | ||
| // Describes when the clients can retry a failed request. Clients could ignore | ||
| // the recommendation here or retry when this information is missing from the error | ||
| // responses. | ||
| // | ||
| // It's always recommended that clients should use exponential backoff when | ||
| // retrying. | ||
| // | ||
| // Clients should wait until `retry_delay` amount of time has passed since | ||
| // receiving the error response before retrying. If retrying requests also | ||
| // fail, clients should use an exponential backoff scheme to increase gradually | ||
| // the delay between retries based on `retry_delay` until either a maximum | ||
| // number of retries has been reached, or a maximum retry delay cap has been | ||
| // reached. | ||
| ```go | ||
| # Do this on the server side. | ||
|
DylanRussell marked this conversation as resolved.
Outdated
|
||
| trailer := metadata.Pairs("grpc-retry-pushback-ms", "5000") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this expected to be done only if the server wants to pushback? It is not clear from the sample if this is some sort of permanent option that the server must always set or only set when the server is overloaded and needs to pushback the clients. |
||
| grpc.SetTrailer(ctx, trailer) | ||
| ``` | ||
|
|
||
| The value of `retry_delay` is determined by the server and is implementation | ||
| dependant. The server SHOULD choose a `retry_delay` value that is big enough to | ||
| On the client side using gRPC retry config (https://grpc.io/docs/guides/retry/) | ||
| will cause gRPC to automatically parse this header and handle all backoff and retry logic. | ||
|
DylanRussell marked this conversation as resolved.
Outdated
|
||
|
|
||
| When the client receives this signal, if it isn't using gRPC retry config, it SHOULD | ||
| follow the recommendations or outlined in documentation for | ||
| [RetryInfo](https://github.com/googleapis/googleapis/blob/6a8c7914d1b79bd832b5157a09a9332e8cbd16d4/google/rpc/error_details.proto#L40). | ||
|
DylanRussell marked this conversation as resolved.
Outdated
|
||
|
|
||
| The value of `retry_delay`/`grpc-retry-pushback-ms` is determined by the server and is implementation | ||
| dependant. The server SHOULD choose a `retry_delay`/`grpc-retry-pushback-ms` value that is big enough to | ||
| give the server time to recover yet is not too big to cause the client to drop | ||
| data while being throttled. | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Please link to a permanent link instead of one that can break.
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.
Isn’t the problem that permanent link may include outdated information?