-
Notifications
You must be signed in to change notification settings - Fork 10
feat: simplify timeout settings #175
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #175 +/- ##
==========================================
+ Coverage 96.72% 96.77% +0.04%
==========================================
Files 18 18
Lines 1436 1457 +21
Branches 191 196 +5
==========================================
+ Hits 1389 1410 +21
Misses 5 5
Partials 42 42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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. I just have a couple of questions about a removed test, and replacing default
with null
in method arguments.
Assert.That(_client, Is.Not.Null); | ||
} | ||
|
||
[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.
I'm not sure we want to remove this test if the ClientConfig.Timeout
property has only been deprecated and not removed. Some users may continue to use it. It's reassuring to know, even though the property is deprecated, the code section continues to work.
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.
I removed this test and the part where we set the Timeout directly to HttpClient object because if we set the Timeout directly to the HttpClient, it will have conflicting behavior when we also set QueryOptions.Deadline.
If we set the Timeout directly to the HttpClient and set the QueryOptions.Deadline, grpc will get the smallest timeout between them, and I don't think this is the right behavior.
The new implementation will absolutely prioritize: QueryOptions.Deadline > QueryTimeout > Timeout (If the users set the Timeout property, it will not directly set the Timeout to the HttpClient, so the QueryOptions.Deadline still runs like intended ). These are my thoughts.
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 OK. Tests passing locally. 🚴 🏁
Issue Link
Proposed Changes
timeout
for write APIs to 10 seconds. Additionally, setting the timeout directly to HttpClient will override the deadline in grpc (or maybe I'm wrong here...),Checklist