Skip to content

Conversation

NguyenHoangSon96
Copy link
Contributor

@NguyenHoangSon96 NguyenHoangSon96 commented Sep 11, 2025

Issue Link

Proposed Changes

  • I have removed the code that sets the timeout directly to HttpClient; now the timeout will be set to the function that makes an HTTP call every time users make a call. I think this makes the logic easier to implement and supports backward compatibility. We already set the default 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...),
  • Query and Write APIs both support setting a one-time "timeout" now, but in a different form, e.g., CancelToken for write APIs, TimeSpan for query APIs.
  • Setting a timeout directly to the query or write functions will have the highest priority now.

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • Tests pass
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@NguyenHoangSon96 NguyenHoangSon96 added this to the 1.4.0 milestone Sep 11, 2025
@NguyenHoangSon96 NguyenHoangSon96 self-assigned this Sep 11, 2025
Copy link

codecov bot commented Sep 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.77%. Comparing base (f95b720) to head (e811c69).
⚠️ Report is 3 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@karel-rehor karel-rehor left a 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]
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@karel-rehor karel-rehor left a 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. 🚴 🏁

@NguyenHoangSon96 NguyenHoangSon96 merged commit 3c8accd into main Sep 15, 2025
10 checks passed
@NguyenHoangSon96 NguyenHoangSon96 deleted the feat/simplify-timeout-settings1 branch September 15, 2025 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants