ryankert01 opened a new pull request, #4104:
URL: https://github.com/apache/iggy/pull/4104
Closes part of #3702.
## The problem
Every connector that talks to a remote system retries, and there is no
shared retry loop, so each one wrote its own. The SDK offers two low-level
primitives that each caller assembles by hand:
```rust
pub fn exponential_backoff(base: Duration, attempt: u32, max_delay:
Duration) -> Duration
// ^^^^^^^ 0-based exponent
```
The parameter is a 0-based exponent, but every retry loop counts attempts
from one, so a caller has to remember to subtract one. Most did not. Of twelve
call sites across seven loops, **eight passed the un-decremented counter**, so
their first retry waited `2 × retry_delay` instead of `retry_delay` —
contradicting every README, which documents that field as the "initial backoff
between retries".
| Call site | Sites | First retry waited |
| --- | --- | --- |
| `meilisearch_sink` | 5 | `2 × base` |
| `HttpRetryMiddleware` (status + network paths) | 2 | `2 × base` |
| `check_connectivity_with_retry` | 1 | `2 × base` |
| `doris_sink`, `s3_sink`, `surrealdb_sink`, `source.rs` | 4 | `base` |
The duplication is not only cosmetic. `HttpRetryMiddleware` classifies
failures on the HTTP status code alone, so any backend that reports failure *in
band* cannot use it — Apache Doris answers a failed Stream Load with HTTP 200
and `{"Status":"Fail"}` in the body, so its sink had to hand-roll the entire
loop.
## What this PR does
- **`retry_async(policy, context, is_transient, op)`** — the retry loop for
anything whose failure surfaces as `Err`. Owns attempt counting, backoff, and
the retry/recovery/give-up logs, so callers no longer each track an attempt
count in order to report one.
- **`RetryPolicy { max_attempts, base_delay, max_delay }`** and
**`retry_backoff(base, retry, max)`**, the single place that knows a 1-based
retry number means `base × 2^(k−1)`.
- Doris and the startup connectivity probe migrate onto `retry_async`.
Meilisearch, S3, SurrealDB, RabbitMQ and the middleware keep their own loops —
they retry on an `Ok(Response)`, carry a deadline, or reconnect between
attempts — but take their timing from `retry_backoff`.
- `exponential_backoff` stays public for out-of-tree plugins and now
documents the trap. Both connector skills teach the new helpers, since the old
guidance is what produced the divergence in the first place.
## Breaking change
`ConnectivityConfig` is removed; it was field-for-field `RetryPolicy`.
```
ConnectivityConfig RetryPolicy
max_open_retries -> max_attempts
retry_delay -> base_delay
open_retry_max_delay -> max_delay
```
Both types are `(u32, Duration, Duration)` and the two delay roles are
crossed, so a mechanical field-by-field rename compiles and silently swaps the
base delay for the cap. No FFI signature or `repr(C)` type changed, so
pre-built plugin `.so` files are unaffected; only source rebuilds are.
## Operator-visible timing
The first retry now waits `retry_delay` rather than twice it for
Meilisearch, the InfluxDB startup probe, and anything behind the HTTP
middleware. Meilisearch moves the most: startup window 17s → 12.5s,
per-operation 7s → 3.5s. A deployment relying on the old window to cover a slow
cold start should raise `max_open_retries`. **Attempt counts are unchanged
everywhere.**
## Tests
`retry.rs` had none. It now covers the attempt budget, the transient
predicate, the base-first timing the divergence kept breaking, and a wiremock
case over `HttpRetryMiddleware` — which had no coverage at all despite being
the path every HTTP connector rides.
## Follow-ups
This is deliberately the first of three. Two further defects were found in
the shared primitives while working on this, and both are separable — they
touch different code, carry different risk, and want different reviewers.
Splitting them keeps the low-risk refactor from waiting behind the contentious
part.
**Follow-up 1 — jitter distribution.** Two defects that defeat jitter
exactly when it matters most, both in pure functions with no API surface:
- `jitter` computes its window as `millis / 5`, which is integer-zero below
5ms, so any sub-5ms delay is returned unchanged and every instance retries on
the same tick. Reachable from configuration.
- Once the exponential saturates, `exponential_backoff` returns exactly
`max_delay`; applying ±20% jitter and clamping back to the cap folds every draw
above it onto one value. Measured over 512 draws, 260 landed on `max_delay` to
the nanosecond — the herd resynchronising precisely when the most instances are
backed off together.
**Follow-up 2 — `Retry-After` handling.** The middleware sleeps a
server-supplied `Retry-After` verbatim with no upper bound, and reads it only
on 429 although RFC 9110 permits it on 503 and InfluxDB Cloud sends it there.
The obvious fix is wrong in an interesting way: clamping the header to
`retry_max_delay` re-hits a throttling server inside its own rate window and
exhausts the attempt budget, and because the sink path is at-most-once that
turns recoverable throttling into a dropped batch. It needs a dedicated
ceiling, and how long a connector may park a worker thread on a server's say-so
is an operational policy decision I would rather settle with maintainers than
assume — so it is not in this PR.
I will open both once this lands, since they build on `retry_backoff`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
