-
Notifications
You must be signed in to change notification settings - Fork 193
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
fix gRPC timeout (#1498) #1499
fix gRPC timeout (#1498) #1499
Conversation
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1499 +/- ##
============================================
+ Coverage 70.76% 70.78% +0.01%
Complexity 2762 2762
============================================
Files 408 408
Lines 8343 8344 +1
============================================
+ Hits 5904 5906 +2
+ Misses 2439 2438 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
LGTM - with no collector available I was able to test that the timeout is accurate for <5s values. Beyond 5s, I got a host lookup error which occurred at 5s if the configured timeout doesn't kick in first. Can you please fix the linting error ( |
Fixes #1498 (grpc timeouts not being respected)
The timeout seems to work correctly for long and short variants of the bug with this code but I wasn't able to create meaningful tests. Whilst it was possible to get the calculated timeval from the transport instance using reflection, actually calling functions on
\Grpc\Timeval
(such as similar() just crashes the test runnerNo additional unit tests fail compared to
main
(35 vs 35)Happy to make any adjustments according to feedback