Core: Retry requests with an Idempotency-Key on retriable errors - #17947
Core: Retry requests with an Idempotency-Key on retriable errors#17947HotSushi wants to merge 2 commits into
Conversation
ExponentialHttpRequestRetryStrategy only applied the idempotent retriable status codes (408, 500, 502, 503, 504) when the HTTP method itself was idempotent. Mutation endpoints are POST, so a request that already carries an Idempotency-Key header - which the server guarantees is safe to replay - was not retried on 502/504 or on 503 without a Retry-After header. Treat a request carrying the Idempotency-Key header as retry-safe for these codes, in both the response and network-exception retry paths. Requests without the header keep the previous conservative behavior.
|
Thanks for finishing the client side of this — the problem statement matches what I'd expect, and the logic looks right. One blocking item: this breaks Nothing is wrong with the production change. That test simulates a server that finalizes a create but responds 503, asserts the 503 surfaces as an exception, and then manually retries with the same key to get the replayed 200. With this change the client retries automatically and the server replays the 200, so the Worth noting that this test is the strongest evidence the feature works, since it exercises the real server-side dedupe in Two non-blocking notes:
Also |
- Update testIdempotentCreateReplayAfterSimulated503: the client now auto-retries a keyed POST on 503 and the server replays the finalized 200, so the call succeeds transparently. Assert success directly instead of expecting a thrown 503 followed by a manual retry. - Add testIdempotentCreateRetryCarriesSameKey pinning the invariant that every transport attempt of a retried keyed POST carries the identical Idempotency-Key. - Clarify the retry-safety comment in ExponentialHttpRequestRetryStrategy (idempotent method OR Idempotency-Key header), which no longer means strict HTTP idempotency.
Thanks for the review @huaxingao, pushed an update:
On the key-lifetime point: you're right, the client doesn't bound retries to the advertised window today. I'll do it in a separate follow-up PR. Done properly it needs the lifetime plumbed into the retry strategy plus elapsed-time tracking, so it's a bigger change and cleaner on its own. |
| simulateFailureOnFirstSuccessByKey = new java.util.concurrent.ConcurrentHashMap<>(); | ||
| // Records the Idempotency-Key value seen on every mutation request, in arrival order. | ||
| private final List<String> observedMutationIdempotencyKeys = | ||
| new java.util.concurrent.CopyOnWriteArrayList<>(); |
There was a problem hiding this comment.
nit: import java.util.concurrent.CopyOnWriteArrayList
| .filter(k -> k.equals(key)) | ||
| .collect(Collectors.toList()); | ||
| assertThat(observedKeys).hasSize(2); | ||
| assertThat(observedKeys.get(0)).isNotNull().isEqualTo(observedKeys.get(1)); |
There was a problem hiding this comment.
Nit: this can't fail. The filter(k -> k.equals(key)) above already guarantees every surviving element equals key, so this compares key to key. hasSize(2) is the real check, so this line can just be dropped.
2 participants
What & why
The REST client's
ExponentialHttpRequestRetryStrategyonly applies the idempotent-retriable status codes (408, 500, 502, 503, 504) when the HTTP method itself is idempotent (Method.isIdempotent). All catalog mutation endpoints arePOST, for whichMethod.isIdempotentisfalse, so a mutation that already carries anIdempotency-Keyheader — which the server guarantees is safe to replay — was not retried on502/504, or on503without aRetry-Afterheader.This completes the client side of the idempotency support added in #14740 (spec: #14196): the client already generated and sent the key, but its own retry logic ignored it. This change lets the key the client sends actually authorize a retry.
Behavior change
Requests without an
Idempotency-Keykeep the previous conservative behavior — the new path only opens up when the safety contract is in place (the client attaches a key only when the server advertisedidempotency-key-lifetimein the config response).Changes
ExponentialHttpRequestRetryStrategy: treat a request carrying theIdempotency-Keyheader as retry-safe for the idempotent-retriable codes, in both the response path (shouldRetryIdempotent) and the network-exception path (retryRequest(HttpRequest, IOException, …)).TestExponentialHttpRequestRetryStrategycases: a keyed POST retries on {429, 503, 500, 502, 504, 408}; an un-keyed POST does not retry on {503, 500, 502, 504, 408}.No public API changes (
ExponentialHttpRequestRetryStrategyis package-private). This is a client-only change and does not modifyopen-api/rest-catalog*.Testing
./gradlew :iceberg-core:test --tests "org.apache.iceberg.rest.TestExponentialHttpRequestRetryStrategy"— passing.Note on scope
Beyond the response-code cases above, this also extends the network-exception retry path to honor the header, so a dropped connection on a keyed POST is retried for the same reason (the server dedupes on replay). This is a deliberate extension for symmetry; happy to split it into a follow-up if reviewers prefer to keep this PR to the response-code path only.
AI assistance
Drafted with AI assistance (code and test scaffolding). The logic was reviewed by the author and verified against the existing retry-strategy tests; the network-exception path extension noted above is the main area worth reviewer attention.