confluentinc / confluentinc/confluent-sql
Retry idempotent GETs on retryable HTTP status codes, not just transport exceptions
- Dominant language
- Python
- Stars
- 6
- Forks
- 1
- Avg merge
- 21h 47m
- Merged PRs (30d)
- 26
Description
`call_with_retries`/`Connection._request_get` (added in #137) only retries `httpx.NetworkError`/`httpx.RemoteProtocolError` — pure transport-layer failures where no HTTP response was ever received. A GET that _does_ get a response, but one carrying a transient server-side status (429 Too Many Requests, 500/502/503/504), is not retried at all today: `_request()` calls `response.raise_for_status()` and converts the resulting `httpx.HTTPStatusError` into an `OperationalError` internally, before `call_with_retries` ever sees an exception type it's configured to catch. `OperationalError` propagates to the caller on the very first such response — the same "flaky Confluent Cloud gateway" incident class #137 was written for, just arriving as a status code instead of a dropped socket.
## Motivating comparison
While extending #137, we reviewed how `confluent-flink-table-api-java-plugin` (the Java Table API client, Confluent's other consumer of these same Flink REST APIs) hardens itself against transient failures. Its central retry wrapper, `DefaultPluginContext.isRetryable(int statusCode)`, treats HTTP 408/429/500/502/503/504 as retryable _in addition to_ raw connection errors (its "code 0" case). Our dbapi driver is currently narrower than the client Confluent already ships against the same API, for the same class of incident. Closing that gap seems worth doing rather than waiting for a field report of a 503 that #137 didn't help with.
## Proposed fix
- Give the retry path visibility into the HTTP status code, not just transport exceptions. `_request()` currently swallows `httpx.HTTPStatusError` into `OperationalError` internally, so the retryable-status check needs to happen before that translation — e.g. `_request_get` could call `_request(url, raise_for_status=False, ...)`, inspect `response.status_code` itself, and retry if it's in a new `DEFAULT_RETRYABLE_STATUS_CODES` set, only calling `response.raise_for_status()` (and thus the existing `OperationalError` translation) once retries are exhausted or the status isn't retryable.
- New constant, e.g. `DEFAULT_RETRYABLE_STATUS_CODES = frozenset({429, 500, 502, 503, 504})`, mirroring `DEFAULT_RETRYABLE_EXCEPTIONS`. Whether 408 belongs is an open call given we already deliberately exclude `httpx.TimeoutException` from the transport-exception set for the same "don't compound latency on a timeout" reasoning — flag for discussion rather than resolving silently either way.
- Stays scoped to the three existing idempotent GET call sites (`list_statements`, `_get_statement`, `_get_statement_results`) — mutating calls remain out of scope, same rationale as #137 (retrying a POST/PATCH/DELETE after an ambiguous response could double-submit or double-mutate state).
- Open question, not necessarily in scope for a first cut: should a `Retry-After` header on a 429/503 response override our own backoff pacing? Worth a note in the PR either way rather than silently ignoring it if present.
## Timeline
Same sequencing as #137/#138: implement first against release `v0.4.x`, ship a followup patch release, then port forward into `main`.
## Testing
- `call_with_retries` (or wherever the status check lands): a mock response object carrying a retryable status code triggers a retry; a non-retryable status code (404, 400) does not.
- Connection-level test: `self._client.request` returns a `Mock` response with `status_code=503` then a normal `200`, on each of the three GET call sites — assert the final result is correct and the retryable-status path was actually exercised (not just the transport-exception path from #137's existing tests).
- A non-retryable status (404 on `_get_statement`) must still surface `StatementNotFoundError` after exactly one call — regression guard against accidentally retrying everything.
- #137's existing transport-exception tests (`test_retry_unit.py`, `TestConnectionRetriesIdempotentGets`) must remain green unchanged.
## Relationships
Related: #137 (introduced `call_with_retries`/`_request_get`), #138 (unwrapped exception translation in `_request` — the two error-translation paths in `_request` are adjacent and worth reviewing together, even though this issue's fix should land before `raise_for_status()` is ever called rather than changing what it translates to).
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.