gardenia opened a new pull request, #18903:
URL: https://github.com/apache/iceberg/pull/18903
## Description
Fixes #18902.
When a REST commit request fails after it has been sent (no response, a
connection reset, a read
timeout), `HTTPClient` wraps the `IOException` in a plain `RESTException`.
The server may have
applied the commit, but the client reports an ordinary failure: it never
raises
`CommitStateUnknownException`, and `RESTTableOperations` never tries the
reconciliation it already
does for 5xx responses. A caller that retries the failed operation then
commits it twice.
This change reports a transport failure on a commit request as
`CommitStateUnknownException`, so it
is handled like a 5xx. Snapshot-add-only commits are reconciled, and other
commits are reported as
unknown rather than failed. A `ConnectException` proves the request was
never sent, so it stays a
`RESTException`. The conversion applies only to commit requests, not to
`HTTPClient` as a whole,
which was the concern raised in #6778.
## Changes
- Add `RESTTableOperations.commitRequest`, which converts a `RESTException`
caused by an
`IOException` (other than `ConnectException`) into a
`CommitStateUnknownException`.
- Use it for table commits in `RESTTableOperations.commit`, so the existing
reconciliation also
covers lost responses.
- Use it for multi-table commits in `RESTSessionCatalog.commitTransaction`.
- Add regression tests in `TestRESTCatalog`:
- `testReconcileOnLostCommitResponse`: the server applies an append but
the response is lost; the
commit is reconciled and succeeds.
- `testLostCommitResponseIsCommitStateUnknown`: a lost response to a
commit that can't be
reconciled raises `CommitStateUnknownException`.
- `testCommitThatCannotConnectIsNotCommitStateUnknown`: a
`ConnectException` stays a
`RESTException`.
- `testLostTransactionResponseIsCommitStateUnknown`: the same for a
multi-table transaction.
## Validation
- Core `TestRESTCatalog` and `TestHTTPClient`: passed (257 tests, on `main`
at ab8a86d).
- Spotless Java formatting: passed.
- `git diff --check`: passed.
- End to end, with Spark 3.5 against `apache/iceberg-rest-fixture` and a
proxy that forwards one
commit and then drops the connection (the reproduction in #18902): with
this change applied to
1.12.0, the `INSERT` reports success and the row is committed once;
without it, the `INSERT`
fails with `RESTException` although the row was committed.
## Notes for reviewers
- Any `IOException` other than `ConnectException` is treated as a
possibly-sent request. Some of
those can happen before the request is sent (for example a TLS handshake
failure). Those now
report an unknown state instead of a failure. That is conservative: the
engine skips cleanup and
may leave orphan files, but it never deletes files a commit references.
- I haven't checked how this interacts with the HTTP client's own retry
strategy. In the end-to-end
run, the commit request reached the catalog only once.
- The end-to-end check used a Spark runtime built from 1.12.0 with this
change, not one built from
`main`.
## AI Disclosure
Model: Claude Opus 5.5
Platform/Tool: Claude Code
Human Oversight: Reviewed the change and tests; reproduced the issue
Prompt Summary: Investigate a lost commit response found by fault-injection
testing against a REST
catalog, reproduce it against `apache/iceberg-rest-fixture`, and fix it so
transport failures on
commit requests are reported as an unknown commit state, with regression
tests.
--
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]
