googleapis / googleapis/google-cloud-dart

Share retry logic between google_cloud_pubsub and google_cloud_storage (before pubsub 1.0)

Open
#346 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.