feat(gax): add client-level attempt_timeout and use as transport default#5630
feat(gax): add client-level attempt_timeout and use as transport default#5630abhinavgautam01 wants to merge 4 commits intogoogleapis:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a client-level attempt_timeout configuration for gRPC and HTTP clients, allowing a default timeout to be set via the ClientBuilder. The implementation merges this default with per-request options, ensuring request-specific settings take precedence. Review feedback highlights opportunities to optimize the merging logic by avoiding unnecessary cloning of RequestOptions through the use of Option::or or std::borrow::Cow, and suggests refactoring duplicated logic into a helper function.
|
ping @dbolduc |
|
/gcbrun |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5630 +/- ##
=======================================
Coverage 97.76% 97.76%
=======================================
Files 221 221
Lines 51115 51156 +41
=======================================
+ Hits 49971 50014 +43
+ Misses 1144 1142 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dbolduc
left a comment
dbolduc
left a comment
There was a problem hiding this comment.
Looks good, thanks! We just need to get past the CI.
I think we need a
cargo fmt -p google-cloud-gax
cargo fmt -p google-cloud-gax-internalAnd then because we added a new API to gax which gax-internal depends on, we need to bump the version of gax.
That happens in:
- Cargo.toml
- src/gax/Cargo.toml
Then we need to update the generated Cargo.lock with like: cargo check -p google-cloud-gax
Add attempt_timeout: Option<std::time::Duration> to internal ClientConfig and ClientBuilder::with_attempt_timeout. gax-internal HTTP and gRPC transports use the client default when no per-request attempt timeout is set.
…ient attempt_timeout Add resolve_effective_timeout helper and use it to merge per-request and client-level attempt_timeouts without cloning RequestOptions. Simplify HTTP and gRPC transport logic and remove redundant cloning. Addresses code-review feedback.
abhinavgautam01
force-pushed
the
feat/add-client-attempt-timeout
branch
from
5dc017d to
5295697
Compare
|
/gcbrun |
2 participants
Fixes #4466
Adds an optional client-level per-attempt timeout and applies it as a transport fallback when requests don't set their own per-attempt timeout.
What changed:
Behavior:
Tests: