googleapis / googleapis/google-cloud-dart
Share retry logic between google_cloud_pubsub and google_cloud_storage (before pubsub 1.0)
- Dominant language
- Dart
- Stars
- 23
- Forks
- 15
- Avg merge
- 1d 9h
- Merged PRs (30d)
- 21
Description
`google_cloud_pubsub` and `google_cloud_storage` each have their own retry
implementation, and the `ServiceException` half of the retryability predicate
(408/429/500/502/503/504) is currently duplicated verbatim between them.
- `pkgs/google_cloud_pubsub/lib/src/retry.dart` — `RetrySettings` data class
(validated, non-const, `==`/`hashCode`), plus `@internal` `delaySequence`,
`isRetryable`, `runWithRetry`. Only `RetrySettings` is exported.
- `pkgs/google_cloud_storage/lib/src/retry.dart` — `sealed RetryRunner` /
`ExponentialRetry` / `NoDelayRetry` / `const defaultRetry`, exported
wholesale and threaded through ~39 public signatures.
Context: https://github.com/googleapis/google-cloud-dart/pull/292
## Suggested order
1. Share the **internals** first (`delaySequence`, `runWithRetry`, and the
`ServiceException` arm of the predicate). This is invisible to users and
needs no public API decision.
2. Only then consider unifying the public settings type.
## Open questions to resolve first
- **Home package.** `package:google_cloud_rpc` is the only package both
already depend on (`^0.6.0`), but it is generated: `lib/rpc.dart` is not in
librarian's `keep` list, so hand-written `lib/retry.dart` +
`lib/src/retry.dart` + tests must each be added to `keep`, and `clock` added
to its dependencies — which makes `clock` transitive for every generated
REST package. It must also stay free of `package:grpc`.
- **`const` vs validation.** `defaultRetry` is a `const` default parameter
value in ~39 storage signatures, so a merged type must have a `const`
constructor — which cannot have a body. But `assert`s cannot express the
`Duration` invariants at all (`assert(initialDelay > Duration.zero)` in a
const constructor is a compile error: *"In constant expressions, operands of
this operator must be of type 'num'"*), and the `int`/`double` asserts that
*can* be written do not run under `dart run` or `dart compile exe`. So
unifying the type means losing eager `ArgumentError` validation unless a
separate `checkValid()` is introduced and called from every non-const
boundary.
- **`NoDelayRetry`.** It is `const ... (initialDelay: Duration.zero,
maxDelay: Duration.zero)`, which pubsub's validation explicitly rejects.
- **Jitter.** pubsub applies ±20%; storage applies none. Unifying changes
storage's runtime retry timing.
- **Defaults must stay per-service.** pubsub 100 ms / ×1.3 / 1 min vs storage
1 s / ×2 / 2 min. Do not share `defaultRetry`.
- **Naming.** pubsub's `totalTimeout` matches GAX; storage's
`maxRetryInterval` reads like a per-attempt cap, which is actually
`maxDelay`.
## Constraint
Move the declaration and re-export it — never introduce a second same-named
type. Two `RetrySettings` declarations would be an ambiguous import for anyone
using both packages.
Until this is done, any fix to retryability must be applied to both copies.
TAG=agy
CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
Contributor guide
Research direction
Start with pkgs/google_cloud_pubsub/lib/src/retry.dart and pkgs/google_cloud_storage/lib/src/retry.dart, then review google_cloud_rpc's generated-file and librarian keep-list constraints. Compare the two retry implementations and resolve the package-home, validation, jitter, naming, and default-behavior questions before changing the public API. Done means the internals are shared without duplicate declarations and service-specific defaults remain intact.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- dart
- Domain
- backend-api-design
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100