huaxingao commented on PR #17947: URL: https://github.com/apache/iceberg/pull/17947#issuecomment-5546665535
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 `TestRESTCatalog.testIdempotentCreateReplayAfterSimulated503`. It passes on the merge base (8ea7d0041) and fails on this branch with "Expecting code to raise a throwable" at TestRESTCatalog.java:3481. 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 `post()` call now succeeds and the `assertThatThrownBy` no longer sees a throwable. The test just encodes the old expectation and needs updating. Worth noting that this test is the strongest evidence the feature works, since it exercises the real server-side dedupe in `CatalogHandlers` rather than the retry strategy in isolation. Could you run `./gradlew :iceberg-core:test --tests "org.apache.iceberg.rest.*"` — the PR description mentions only `TestExponentialHttpRequestRetryStrategy`, which is why this wasn't caught. That's the only failure among the 877 tests in that package. Two non-blocking notes: 1. The safety of this change rests entirely on the Idempotency-Key being identical across retry attempts, and nothing tests that. I verified it does hold today (`BaseHTTPClient.post` resolves the header supplier once, and HttpClient retries the same request object), but if someone later moved header resolution into a request interceptor, every retry would mint a new key and silently produce duplicate commits with no test failing. A test asserting both attempts carry the same key would pin the invariant. 2. `idempotency-key-lifetime` is only null-checked to enable keys; the duration is never read, and nothing in the retry path is time-bounded. The spec puts this obligation on the client: "Clients SHOULD NOT reuse an Idempotency-Key after this window elapses." Defaults are fine (max-retries 5 is ~31s of backoff against PT30M), but `getRetryInterval` honors `Retry-After` verbatim with no cap and the retry count is configurable, so a key can be replayed past the advertised window where the server may no longer dedupe. Fine as a follow-up, just want it to be a conscious decision. Also `shouldRetryIdempotent` and its "Check if the request is idempotent" comment now describe retry-safety rather than idempotency, so the name has drifted. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
