feat(spanner): export SpannerRetryPolicy#6048
Conversation
There was a problem hiding this comment.
Code Review
This pull request makes the SpannerRetryPolicy and its module public, and adds detailed documentation along with a usage example. The feedback highlights a critical issue where linking to the privately imported Aip194Strict in public documentation will trigger a rustdoc warning and fail the CI build. Fully qualifying the link to google_cloud_gax::retry_policy::Aip194Strict is recommended to resolve this.
The Spanner client installs `SpannerRetryPolicy` as the default retry policy for RPCs that do not configure their own, but the type was `pub(crate)`. Downstream users who wanted to *bound* the client's default behavior (e.g. cap attempts or elapsed time with `RetryPolicyExt::with_attempt_limit` / `with_time_limit`) while keeping the Spanner-specific error classification — retrying transport/IO errors on idempotent requests on top of the strict AIP-194 checks — had to maintain a byte-for-byte behavioral copy of the policy, which can silently drift when the crate changes. Make the `retry_policy` module and `SpannerRetryPolicy` (+ its `new` constructor) public, with rustdoc describing the policy's behavior and an example decorating it with `RetryPolicyExt`. No behavior changes. Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
ee82950 to
cbdc970
Compare
|
/gcbrun |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6048 +/- ##
=======================================
Coverage 96.95% 96.95%
=======================================
Files 248 248
Lines 62379 62379
=======================================
+ Hits 60480 60482 +2
+ Misses 1899 1897 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request exposes the retry_policy module and the SpannerRetryPolicy struct publicly, including its new constructor, and adds comprehensive documentation with usage examples. The feedback points out an unresolved rustdoc reference link in the newly added documentation that should be updated to standard inline link syntax to avoid warnings during documentation builds.
|
@olavloite Thanks for the review! If you have the time, please review #6037 as well - it's a major performance hit with many rows, with a simple and safe fix. |
…port (UP-3) googleapis/google-cloud-rust#6048 (merged as 367b678) makes the Spanner client's `retry_policy` module and `SpannerRetryPolicy::new()` public, with rustdoc naming exactly our use case: decorate the policy with `RetryPolicyExt` instead of re-implementing its error classification. That removes the reason for the driver's behavioural copy of the then-private type. `RetryConfig::retry_policy_arg` now bounds the client's own `SpannerRetryPolicy::new()`, so the copy can no longer silently drift from the client on a rev bump — the drift risk UP-3 was filed for is gone by construction. No behaviour change: the exported type is the same Aip194Strict + transport/IO-on-idempotent decoration the copy implemented. Bumps the google-cloud-rust pin to deac9e2f (current upstream main, which includes #6048) across all eight crates of the family in lockstep, per the CLAUDE.md revert checklist. The `base_policy_retries_io_errors_only_when_idempotent` unit test is kept, now exercising the client's type: upstream tests it too, but that behaviour is precisely why the driver re-applies the client's policy rather than a bare `Aip194Strict`, so it stays as a rev-bump guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EFgnryaZJKj1Tn68nBfkGb
…port (UP-3) googleapis/google-cloud-rust#6048 (merged as 367b678) makes the Spanner client's `retry_policy` module and `SpannerRetryPolicy::new()` public, with rustdoc naming exactly our use case: decorate the policy with `RetryPolicyExt` instead of re-implementing its error classification. That removes the reason for the driver's behavioural copy of the then-private type. `RetryConfig::retry_policy_arg` now bounds the client's own `SpannerRetryPolicy::new()`, so the copy can no longer silently drift from the client on a rev bump — the drift risk UP-3 was filed for is gone by construction. No behaviour change: the exported type is the same Aip194Strict + transport/IO-on-idempotent decoration the copy implemented. Bumps the google-cloud-rust pin to deac9e2f (current upstream main, which includes #6048) across all eight crates of the family in lockstep, per the CLAUDE.md revert checklist. The `base_policy_retries_io_errors_only_when_idempotent` unit test is kept, now exercising the client's type: upstream tests it too, but that behaviour is precisely why the driver re-applies the client's policy rather than a bare `Aip194Strict`, so it stays as a rev-bump guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EFgnryaZJKj1Tn68nBfkGb
…port (UP-3) googleapis/google-cloud-rust#6048 (merged as 367b678) makes the Spanner client's `retry_policy` module and `SpannerRetryPolicy::new()` public, with rustdoc naming exactly our use case: decorate the policy with `RetryPolicyExt` instead of re-implementing its error classification. That removes the reason for the driver's behavioural copy of the then-private type. `RetryConfig::retry_policy_arg` now bounds the client's own `SpannerRetryPolicy::new()`, so the copy can no longer silently drift from the client on a rev bump — the drift risk UP-3 was filed for is gone by construction. No behaviour change: the exported type is the same Aip194Strict + transport/IO-on-idempotent decoration the copy implemented. Bumps the google-cloud-rust pin to deac9e2f (current upstream main, which includes #6048) across all eight crates of the family in lockstep, per the CLAUDE.md revert checklist. The `base_policy_retries_io_errors_only_when_idempotent` unit test is kept, now exercising the client's type: upstream tests it too, but that behaviour is precisely why the driver re-applies the client's policy rather than a bare `Aip194Strict`, so it stays as a rev-bump guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EFgnryaZJKj1Tn68nBfkGb
…port (UP-3) googleapis/google-cloud-rust#6048 (merged as 367b678) makes the Spanner client's `retry_policy` module and `SpannerRetryPolicy::new()` public, with rustdoc naming exactly our use case: decorate the policy with `RetryPolicyExt` instead of re-implementing its error classification. That removes the reason for the driver's behavioural copy of the then-private type. `RetryConfig::retry_policy_arg` now bounds the client's own `SpannerRetryPolicy::new()`, so the copy can no longer silently drift from the client on a rev bump — the drift risk UP-3 was filed for is gone by construction. No behaviour change: the exported type is the same Aip194Strict + transport/IO-on-idempotent decoration the copy implemented. Bumps the google-cloud-rust pin to deac9e2f (current upstream main, which includes #6048) across all eight crates of the family in lockstep, per the CLAUDE.md revert checklist. The `base_policy_retries_io_errors_only_when_idempotent` unit test is kept, now exercising the client's type: upstream tests it too, but that behaviour is precisely why the driver re-applies the client's policy rather than a bare `Aip194Strict`, so it stays as a rev-bump guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EFgnryaZJKj1Tn68nBfkGb
…port (UP-3) googleapis/google-cloud-rust#6048 (merged as 367b678) makes the Spanner client's `retry_policy` module and `SpannerRetryPolicy::new()` public, with rustdoc naming exactly our use case: decorate the policy with `RetryPolicyExt` instead of re-implementing its error classification. That removes the reason for the driver's behavioural copy of the then-private type. `RetryConfig::retry_policy_arg` now bounds the client's own `SpannerRetryPolicy::new()`, so the copy can no longer silently drift from the client on a rev bump — the drift risk UP-3 was filed for is gone by construction. No behaviour change: the exported type is the same Aip194Strict + transport/IO-on-idempotent decoration the copy implemented. Bumps the google-cloud-rust pin to deac9e2f (current upstream main, which includes #6048) across all eight crates of the family in lockstep, per the CLAUDE.md revert checklist. The `base_policy_retries_io_errors_only_when_idempotent` unit test is kept, now exercising the client's type: upstream tests it too, but that behaviour is precisely why the driver re-applies the client's policy rather than a bare `Aip194Strict`, so it stays as a rev-bump guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EFgnryaZJKj1Tn68nBfkGb
…port (UP-3) (#319) googleapis/google-cloud-rust#6048 (merged as 367b678) makes the Spanner client's `retry_policy` module and `SpannerRetryPolicy::new()` public, with rustdoc naming exactly our use case: decorate the policy with `RetryPolicyExt` instead of re-implementing its error classification. That removes the reason for the driver's behavioural copy of the then-private type. `RetryConfig::retry_policy_arg` now bounds the client's own `SpannerRetryPolicy::new()`, so the copy can no longer silently drift from the client on a rev bump — the drift risk UP-3 was filed for is gone by construction. No behaviour change: the exported type is the same Aip194Strict + transport/IO-on-idempotent decoration the copy implemented. Bumps the google-cloud-rust pin to deac9e2f (current upstream main, which includes #6048) across all eight crates of the family in lockstep, per the CLAUDE.md revert checklist. The `base_policy_retries_io_errors_only_when_idempotent` unit test is kept, now exercising the client's type: upstream tests it too, but that behaviour is precisely why the driver re-applies the client's policy rather than a bare `Aip194Strict`, so it stays as a rev-bump guard. Claude-Session: https://claude.ai/code/session_01EFgnryaZJKj1Tn68nBfkGb Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Spanner client installs
SpannerRetryPolicyas the default retry policy for RPCs that do not configure their own, but the type waspub(crate).Downstream users who wanted to bound the client's default behavior (e.g. cap attempts or elapsed time with
RetryPolicyExt::with_attempt_limit/with_time_limit) while keeping the Spanner-specific error classification - retrying transport/IO errors on idempotent requests on top of the strict AIP-194 checks - had to maintain a byte-for-byte behavioral copy of the policy, which can silently drift when the crate changes.Make the
retry_policymodule andSpannerRetryPolicy(+ itsnewconstructor) public, with rustdoc describing the policy's behavior and an example decorating it withRetryPolicyExt. No behavior changes.Precedent in other language clients
The other official Spanner clients treat the default retry configuration as public, user-tunable API rather than an internal detail; this PR brings the Rust preview client in line.
cloud.google.com/go/spanner): the Spanner-specific default backoff is an exported package variable,spanner.DefaultRetryBackoff, and the handwritten client'sClientConfig.CallOptions *vkit.CallOptionsexists precisely "for providing custom retry settings that override the default values". The generatedapiv1.Clientadditionally exposes itsCallOptionsfield publicly, pre-populated with the per-RPC default retryers (spanner/apiv1/spanner_client.go), so callers can inspect and rebound the defaults directly.googleapis/java-spanner):SpannerOptions.Builder.getSpannerStubSettingsBuilder()is public and documented for setting customRetrySettingsper gRPC method (including the documentedapplyToAllUnaryMethodsidiom); the builder starts from the public generatedSpannerStubSettingsdefaults, so the default per-methodRetrySettingsare obtainable via public API (e.g.executeSqlSettings().getRetrySettings()) and can be modified withtoBuilder()rather than re-implemented.googleapis/python-spanner): every generated and handwritten data method takes a publicretry=parameter accepting agoogle.api_core.retry.Retry(e.g.Snapshot.execute_sql), and the handwritten layer exports a module-level default,DEFAULT_RETRY_BACKOFF = Retry(initial=0.02, maximum=32, multiplier=1.3)inspanner_v1/database.py. One nuance: the per-RPC generated defaults live asdefault_retryvalues inside the generated transport (transports/base.py) — visible source but not exported constants — though any of them can be replaced per call.googleapis/nodejs-spanner): the default per-method retry codes and backoff parameters ship as data in the published package (src/v1/spanner_client_config.json), and the generated client's constructor accepts a documentedclientConfigoverride merged over those defaults via gaxconstructSettings, plus per-call gaxCallOptionsretry settings (src/v1/spanner_client.ts). The default here is packaged data rather than an exported symbol, but it is still user-visible and overridable without re-implementation.