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.

Sign up for GitHub

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

Jump to bottom

client: implement maxAttempts for retryPolicy #7229

Merged
merged 7 commits into from
May 24, 2024
Merged

client: implement maxAttempts for retryPolicy #7229

merged 7 commits into from
May 24, 2024
+129 −13

Conversation

imoore76
Copy link
Contributor

@imoore76 imoore76 commented May 12, 2024 •
edited by dfawley
Loading

Fixes #4615

RELEASE NOTES:

  • grpc: add WithMaxCallAttempts to configure gRPC's retry behavior per-channel.

@imoore76
Copy link
Contributor Author

Hi @aranjans! Is there something you are waiting for from me?

@aranjans
Copy link
Contributor

@imoore76 no, i think this PR LGTM! Approved.

@dfawley dfawley assigned dfawley and unassigned aranjans May 21, 2024
dfawley
dfawley reviewed May 21, 2024
View reviewed changes
service_config.go Show resolved Hide resolved
dialoptions.go Outdated Show resolved Hide resolved
test/retry_test.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned imoore76 and unassigned dfawley May 21, 2024
@dfawley dfawley added the Type: Feature New features or improvements in behavior label May 21, 2024
@dfawley dfawley added this to the 1.65 Release milestone May 21, 2024
@imoore76 imoore76 requested a review from dfawley May 22, 2024 10:09
dfawley
dfawley reviewed May 23, 2024
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for the PR. Just one thing that needs to change: the godoc comment, and one thing that would be nice-to-have (combining test cases) if it's possible, and one small optional thing that is a judgment call that I'll leave up to you.

dialoptions.go Outdated Show resolved Hide resolved
service_config.go Show resolved Hide resolved
test/retry_test.go Outdated Show resolved Hide resolved
test/retry_test.go Outdated Show resolved Hide resolved
test/retry_test.go Outdated Show resolved Hide resolved
@dfawley
Copy link
Member

dfawley commented May 24, 2024

Thank you for the PR!

@dfawley dfawley merged commit 03da31a into grpc:master May 24, 2024
12 checks passed
@No-SilverBullet No-SilverBullet mentioned this pull request Jul 4, 2024
Closed
mtardy added a commit to cilium/tetragon that referenced this pull request Jul 23, 2024
@mtardy
tetra: use the builtin gRPC retry backoff mechanism
a696b32 
gRGC A6 - gRPC Retry Design (a.k.a. built in backoff retry)
https://github.com/grpc/proposal/blob/master/A6-client-retries.md was
implemented by grpc/grpc-go#2111 but unusable
for a long time since maxAttempts was limited to hardcoded 5
(grpc/grpc-go#4615), recent PR fixed that
grpc/grpc-go#7229.

It's transparent to the user, to see it in action, make sure the gRPC
server is unreachable (do not start tetragon for example), run tetra
with: GRPC_GO_LOG_SEVERITY_LEVEL=warning <tetra cmd>

Note that logs don't always have the time to be pushed before exit so
output might be a bit off but the number of retries is respected (you
can debug or synchronously print in the grpc/stream.c:shouldRetry or
:withRetry to verify).

Also note that the final backoff duration is completely random and
chosen between 0 and the final duration that was computed via to the
params: https://github.com/grpc/grpc-go/blob/v1.65.0/stream.go#L702

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
mtardy added a commit to cilium/tetragon that referenced this pull request Jul 29, 2024
@mtardy
tetra: use the builtin gRPC retry backoff mechanism
06b85c9 
gRGC A6 - gRPC Retry Design (a.k.a. built in backoff retry)
https://github.com/grpc/proposal/blob/master/A6-client-retries.md was
implemented by grpc/grpc-go#2111 but unusable
for a long time since maxAttempts was limited to hardcoded 5
(grpc/grpc-go#4615), recent PR fixed that
grpc/grpc-go#7229.

It's transparent to the user, to see it in action, make sure the gRPC
server is unreachable (do not start tetragon for example), run tetra
with: GRPC_GO_LOG_SEVERITY_LEVEL=warning <tetra cmd>

Note that logs don't always have the time to be pushed before exit so
output might be a bit off but the number of retries is respected (you
can debug or synchronously print in the grpc/stream.c:shouldRetry or
:withRetry to verify).

Also note that the final backoff duration is completely random and
chosen between 0 and the final duration that was computed via to the
params: https://github.com/grpc/grpc-go/blob/v1.65.0/stream.go#L702

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
mtardy added a commit to cilium/tetragon that referenced this pull request Jul 29, 2024
@mtardy
tetra: use the builtin gRPC retry backoff mechanism
f5a92b6 
gRGC A6 - gRPC Retry Design (a.k.a. built in backoff retry)
https://github.com/grpc/proposal/blob/master/A6-client-retries.md was
implemented by grpc/grpc-go#2111 but unusable
for a long time since maxAttempts was limited to hardcoded 5
(grpc/grpc-go#4615), recent PR fixed that
grpc/grpc-go#7229.

It's transparent to the user, to see it in action, make sure the gRPC
server is unreachable (do not start tetragon for example), run tetra
with: GRPC_GO_LOG_SEVERITY_LEVEL=warning <tetra cmd>

Note that logs don't always have the time to be pushed before exit so
output might be a bit off but the number of retries is respected (you
can debug or synchronously print in the grpc/stream.c:shouldRetry or
:withRetry to verify).

Also note that the final backoff duration is completely random and
chosen between 0 and the final duration that was computed via to the
params: https://github.com/grpc/grpc-go/blob/v1.65.0/stream.go#L702

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
mtardy added a commit to cilium/tetragon that referenced this pull request Jul 29, 2024
@mtardy
tetra: use the builtin gRPC retry backoff mechanism
ebae77f 
gRGC A6 - gRPC Retry Design (a.k.a. built in backoff retry)
https://github.com/grpc/proposal/blob/master/A6-client-retries.md was
implemented by grpc/grpc-go#2111 but unusable
for a long time since maxAttempts was limited to hardcoded 5
(grpc/grpc-go#4615), recent PR fixed that
grpc/grpc-go#7229.

It's transparent to the user, to see it in action, make sure the gRPC
server is unreachable (do not start tetragon for example), run tetra
with: GRPC_GO_LOG_SEVERITY_LEVEL=warning <tetra cmd>

Note that logs don't always have the time to be pushed before exit so
output might be a bit off but the number of retries is respected (you
can debug or synchronously print in the grpc/stream.c:shouldRetry or
:withRetry to verify).

Also note that the final backoff duration is completely random and
chosen between 0 and the final duration that was computed via to the
params: https://github.com/grpc/grpc-go/blob/v1.65.0/stream.go#L702

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@aranjans aranjans aranjans approved these changes

@dfawley dfawley dfawley approved these changes

Assignees

@imoore76 imoore76

Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Milestone
1.65 Release
Development

Successfully merging this pull request may close these issues.

Retry: grpc maxretry attempts is hardcoded
3 participants
@imoore76 @aranjans @dfawley