Repository navigation
Conversation
4c0fc66 to
83b7a9d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Uncapped exponential backoff can block application startup for days, while overly broad exception retries and lost Retry-After handling introduce additional retry regressions.
3 open issues
What changed in this PR
This PR moves feature-fetch retries into a shared SDK policy used by both synchronous and asynchronous clients, leaving transports responsible for individual requests.
Changes:
- Centralizes retry decisions and adds exponential backoff with jitter.
- Updates both API clients to use the shared retry helpers.
- Adds retry-policy tests, verifies single-attempt transport behavior, and documents the change.
| File | Description |
|---|---|
UnleashClient/_unleash_api_client.py |
Routes synchronous feature fetches through shared retries. |
UnleashClient/_transport.py |
Removes transport-level retry configuration. |
UnleashClient/_retry.py |
Introduces shared retry decisions and backoff helpers. |
UnleashClient/_async_unleash_api_client.py |
Replaces its retry loop with the shared async helper. |
tests/unit_tests/test_transport.py |
Verifies transports send requests only once. |
tests/unit_tests/test_retry.py |
Covers retry outcomes, cancellation, and backoff. |
tests/conftest.py |
Disables retry delays by default during tests. |
CHANGELOG.md |
Documents shared retries and randomized backoff. |
Review effort: Balanced
| ``BACKOFF_BASE * 2 ** retry``, so 0.25-0.5s, 0.5-1s, 1-2s, and clients that | ||
| failed together don't retry together. | ||
| """ | ||
| ceiling = BACKOFF_BASE * 2**retry |
There was a problem hiding this comment.
i removed the option to configure retry count in a later PR, because I though retry should be the SDK's job, not sth the users decide
said that, we have no way of identifying is a custom transport implementation has retry mechanism, and then it can stack up with out implementation, but i would like to trust the SDK users to not mess that up
there is a warning for that in the readme, and also if they use our RequestTransport class we do log a warning about the duplicate retries
maybe makes sense to allow the users to disable the built-in retry if they really need to, but I would wait for them to explicitly ask for it. then, I have a feeling that it might be better off in the polling options then in the transport
| if isinstance(outcome, Response): | ||
| return outcome.status in RETRY_STATUSES |
There was a problem hiding this comment.
this is actually true for the sync client, not the async, so not sure how much this was intented in the first place
so leaning towards not caring about this one @ppicom wdyt?
| return False | ||
| if isinstance(outcome, Response): | ||
| return outcome.status in RETRY_STATUSES | ||
| return not isinstance(outcome, AlreadyClosedError) |
There was a problem hiding this comment.
we have no way of telling a transient error apart from a permanent one, since custom transport can use any lib they want
but i don't think it is a huge problem since the worst case is 3 extra attempts now for a permanent error
might be worth adding a few non-retryable errors, i am think about adding it in a separate PR
moved retries out of the transports into one shared file in the SDK. retries should be the responsibility of the SDK imo, httpOptions retries will go away
same type of errors should be retried as before but added backoff for retries