Conversation
A transaction whose producers staged no snapshot skipped the refresh, so the rebuild validated against stale metadata and missed concurrent conflicts. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Run the post-commit cleanup once after the loop, flatten the landed check, drop a redundant local import, and trim test setup and docstrings. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4021
Closes #4022
Rationale for this change
When
commit_tableraisesCommitFailedExceptionafter the catalog has already applied the commit (a lost response),Transaction.commit_transactioncan delete the landed snapshot's manifest list and manifests, which leaves the table unreadable:commit.retry.num-retries=0, or after the total timeout), the cleanup runs without checking whether the attempt landed.ValidationExceptioncleans up.We ran into both bugs in Polars'
sink_iceberg, which commits through pyiceberg, and work around them in pola-rs/polars#29511. The workaround wraps the catalog'scommit_table, reloads the table after a failed commit, and returns a success response when a snapshot it sent is present, so pyiceberg never reaches the cleanup. This PR moves that check intocommit_transaction, and Polars can drop the wrapper once it's released.This change:
AddSnapshotUpdatesnapshot ids that were actually sent to the catalog. Each attempt is atomic, so finding any of them proves an attempt landed;CommitFailedException, including on the last attempt, before any cleanup;CommitStateUnknownException(chained to the original error) if the refresh for the check fails. That exception skips the cleanup, so files a landed snapshot may reference are never deleted when the outcome can't be verified;Are these changes tested?
Yes, in
tests/table/test_commit_retry.py, parametrized over the existing catalog fixture:Each asserts that exactly one snapshot exists, that its manifest list is still on disk and that the table scans. All of them fail on
main.Are there any user-facing changes?
A lost response on the final attempt now returns successfully when the commit landed, instead of raising
CommitFailedException. If the landed check itself can't reach the catalog,CommitStateUnknownExceptionis raised instead ofCommitFailedException.Generated-by: Claude Code (Claude Opus 5.5)