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

stream: add jitter to retry backoff in accordance with gRFC A6 #7869

Merged
merged 2 commits into from
Nov 26, 2024

Conversation

isgj
Copy link
Contributor

@isgj isgj commented Nov 24, 2024

Update the delay calculation to reflect the updated gRFC

Address #7514

RELEASE NOTES:

  • client: update retry attempt backoff to apply jitter per updates to gRFC A6.

Update the delay calculation to reflect the updated gRFC
Copy link

linux-foundation-easycla bot commented Nov 24, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Nov 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.00%. Comparing base (13d5a16) to head (df8dd0d).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7869      +/-   ##
==========================================
- Coverage   82.04%   82.00%   -0.05%     
==========================================
  Files         375      375              
  Lines       37984    37984              
==========================================
- Hits        31165    31148      -17     
- Misses       5532     5540       +8     
- Partials     1287     1296       +9     
Files with missing lines Coverage Δ
service_config.go 81.54% <100.00%> (ø)
stream.go 81.69% <100.00%> (+0.25%) ⬆️

... and 15 files with indirect coverage changes

---- 🚨 Try these New Features:

@aranjans aranjans added Type: Behavior Change Behavior changes not categorized as bugs Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. labels Nov 25, 2024
@aranjans aranjans added this to the 1.69 Release milestone Nov 25, 2024
@aranjans
Copy link
Contributor

@isgj Thanks for raising the PR. Can you add an end to end test to verify the same?

stream.go Outdated Show resolved Hide resolved
@aranjans
Copy link
Contributor

Would be good if we could update the README of examples for retry feature.

@arjan-bal
Copy link
Contributor

Would be good if we could update the README of examples for retry feature.

@aranjans can you point to the README and the example file that needs updating?

@arjan-bal arjan-bal assigned aranjans and isgj and unassigned isgj Nov 25, 2024
@isgj
Copy link
Contributor Author

isgj commented Nov 25, 2024

Would be good if we could update the README of examples for retry feature.

The README already points to the right gRFC https://github.com/grpc/grpc-go/blob/master/examples/features/retry/README.md?plain=1#L7


Can you add an end to end test to verify the same?

I see the tests at https://github.com/grpc/grpc-go/blob/master/test/retry_test.go.
Are you thinking about any specific test? I can try to add a test that checks the time when the retry happens, but that kind of test might be flaky

@aranjans
Copy link
Contributor

aranjans commented Nov 26, 2024

@isgj Yes that's the correct reference to README.

Are you thinking about any specific test? I can try to add a test that checks the time when the retry happens, but that kind of test might be flaky
Ah yeah, that might be flaky. We could skip writing the test for that.

Overall this change LGTM, adding the @easwars as second reviewer to take a look.

@aranjans aranjans assigned easwars and unassigned aranjans Nov 26, 2024
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.

Thank you for the PR!

@dfawley dfawley changed the title stream: update retry delay stream: add jitter to retry backoff in accordance with gRFC A6 Nov 26, 2024
@dfawley dfawley merged commit 4c07bca into grpc:master Nov 26, 2024
13 checks passed
@isgj isgj deleted the stream/retry branch November 27, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants