Repository navigation
feat(gax): add client-level attempt_timeout and use as transport default - #5630
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 50013 +42
+ Misses 1144 1143 -1 ☔ 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.
5dc017d to
5295697
Compare
|
/gcbrun |
|
ping @dbolduc |
@coryan is right. The version should be google-cloud-rust/librarian.yaml Line 829 in 04350be Can you fix that? |
Minor version for breaking-API-leaning change; sync workspace, Cargo.lock, and librarian.yaml.
done..! |
|
/gcbrun |
dbolduc
left a comment
dbolduc
left a comment
There was a problem hiding this comment.
LGTM, thanks! Let's see what the builds say.
hopefully the builds stay green...🤧 |
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: