Anurag-S1ngh opened a new pull request, #4339:
URL: https://github.com/apache/iceberg-python/pull/4339
# Rationale for this change
`Transaction.commit_transaction` only checked whether a failed attempt had
actually landed before starting the *next* attempt. When the failure ended the
loop instead, it raised straight into the handler that calls
`_clean_all_uncommitted()` on every producer:
- `commit.retry.num-retries=0`, so there is no next attempt.
- A response lost on the final attempt.
- The total retry timeout elapsing.
A catalog can apply a commit and still report `CommitFailedException`, for
example when the response is lost or a transport retry gets a conflict for a
request that already applied. The snapshot is then live while its manifest list
and manifests have been deleted, so the table can no longer be scanned.
The landed check now runs before the cleanup on the last attempt too. It was
also pulled into `_attempt_landed()`, which refreshes the table and looks for
the producer's snapshot id, so both paths use the same check. If the refresh
fails, the outcome is unknown, so it raises `CommitStateUnknownException` and
the files are kept, which is how other unknown outcomes are already handled.
# Are these changes tested?
Yes, in `tests/table/test_commit_retry.py`, by stubbing
`Catalog.commit_table` to fail before ("conflict") or after ("lost") the real
commit:
- `test_lost_response_on_the_last_attempt_keeps_the_landed_snapshot` uses
`commit.retry.num-retries=0`, appends, and asserts the append returns, exactly
one snapshot exists, its manifest list is still on disk, and the rows scan back.
- `test_lost_response_on_the_final_retry_keeps_the_landed_snapshot` loses
the response on the second and last attempt after a real conflict on the first,
and asserts the same.
- `test_unreadable_commit_outcome_raises_commit_state_unknown` makes the
refresh fail and asserts `CommitStateUnknownException`.
All three fail on main and pass with the change.
Full unit suite: `4575 passed, 5 skipped`. `prek run -a` passes.
# Are there any user-facing changes?
Yes. A commit whose response is lost after the catalog applied it no longer
deletes the files of the snapshot it landed, so the table stays readable, and
the call returns successfully instead of raising. When the outcome cannot be
determined because the table cannot be refreshed, `CommitStateUnknownException`
is raised and no files are deleted.
Closes #4021
--
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]
