-
Notifications
You must be signed in to change notification settings - Fork 840
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
Retry ConnectException, add retry logging #6614
Retry ConnectException, add retry logging #6614
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6614 +/- ##
============================================
- Coverage 90.19% 90.08% -0.12%
+ Complexity 6281 6277 -4
============================================
Files 697 697
Lines 18903 18944 +41
Branches 1852 1858 +6
============================================
+ Hits 17050 17065 +15
- Misses 1281 1305 +24
- Partials 572 574 +2 ☔ View full report in Codecov by Sentry. |
…y-java into retry-connect-exception
@open-telemetry/java-approvers would be great to get this approved and merged before this week's release. |
.isTrue()); | ||
|
||
verify(mockHttpClient, times(2)).send(any(), any()); | ||
} |
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.
😎 solid test!
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.
Looks good to me.
Fixes #6587.
I figured out how to reliably reproduce the
java.net.ConnectException
from the issue by connecting an unused port onlocalhost
. JdkHttpSender was already retrying on these, but OkHttpSender and OkGrpcSender were not.Also added some overdue debug logging for the intermediate retry requests so users have a way of determining what is going on.