Skip to content

Core: Retry appends on catalogs that only expose the current snapshot - #4333

Open
Anurag-S1ngh wants to merge 1 commit into
apache:mainfrom
Anurag-S1ngh:fix/4327-retry-without-starting-snapshot
Open

Anurag-S1ngh wants to merge 1 commit into
apache:mainfrom
Anurag-S1ngh:fix/4327-retry-without-starting-snapshot

Conversation

@Anurag-S1ngh

Copy link
Copy Markdown

Rationale for this change

Some REST catalogs expose only the current snapshot and keep table history elsewhere, Nessie being the main example. CommitWindow.resolve raised ValidationException: Cannot find starting snapshot <id> whenever the starting snapshot was missing after a refresh, so since the retry added in #3320 no commit loser could ever retry on those catalogs, not even a plain append.

The window now records that the starting snapshot is missing instead of raising:

  • Operations that need no conflict validation, such as appends, retry as they do anywhere else. _refresh_for_retry already rebuilds on the refreshed head, which is also what Java's SnapshotProducer does.
  • Operations that do validate, such as overwrites and deletes, cannot run the check without the history. They now raise CommitFailedException with a message saying the starting snapshot is not exposed, which is the pre-0.12 behavior and tells a lost race apart from a data conflict. They never continue without validation.

Are these changes tested?

Yes. New tests in tests/table/test_commit_retry.py hide the starting snapshot from the refreshed metadata to emulate a catalog without history:

  • test_append_retry_without_starting_snapshot races two appends and asserts the loser retries and both land.
  • test_delete_retry_without_starting_snapshot_raises_commit_failed asserts a delete raises CommitFailedException instead of ValidationException.
  • test_validate_concurrency_marks_missing_starting_snapshot covers the window flags directly.

All three fail on main and pass with the change. Full unit suite: 4570 passed, 5 skipped. prek run -a passes.

Are there any user-facing changes?

Yes. On catalogs that only expose the current snapshot, retried appends now succeed instead of raising ValidationException. Retried overwrites and deletes raise CommitFailedException again, as they did before 0.12, with a message that names the missing starting snapshot.

Closes #4327

@AP24110011695 AP24110011695 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified locally (Windows, Python 3.12):

  • Full tests/table/test_commit_retry.py passes on this branch — 129 passed, including the three new history-less-catalog tests across all catalog fixtures.
  • Fail-first reproduced: swapping only the pyiceberg/ source to main makes exactly those new tests fail (9 failed across fixtures) with the ValidationException behavior from #4327 — so the tests genuinely pin the regression.
  • Traced the class structure to confirm the fix lands in the right place: the base_missing guard sits in _SnapshotProducer._validate_concurrency, and _FastAppendFiles overrides that method with a no-op, so appends retry without validation (matching Java's SnapshotProducer behavior) while deletes/overwrites inherit the guarded path and raise CommitFailedException with a message that names the missing snapshot. The only new base=None case (base_id set but snapshot missing) is intercepted before starting_snapshot is used; the pre-existing base_id=None path with a non-empty window is untouched.
  • resolve() no longer raises, is_empty() docstring matches the new semantics, and the error text distinguishes a lost race from a data conflict — the pre-0.12 behavior, restored.

Good catch on a real 0.12 regression for Nessie-style catalogs, and the test design (hiding the starting snapshot from refreshed metadata) is a neat way to emulate them. Nice work.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Commit retry (0.12) always fails on catalogs that expose only the current snapshot (e.g. Nessie REST): "Cannot find starting snapshot", even for appends

2 participants