Skip to content

[core] Treat a snapshot rename that throws but succeeded as committed - #10324

Merged
JingsongLi merged 1 commit into
apache:masterfrom
dev-donghwan:fix-retry-commit-hint
Oct 2, 2026
Merged

JingsongLi merged 1 commit into
apache:masterfrom
dev-donghwan:fix-retry-commit-hint

Conversation

@dev-donghwan

@dev-donghwan dev-donghwan commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Related: #10323 makes the table recover from a stale LATEST hint, whatever made it stale.

Purpose

When a snapshot rename actually succeeds but the file system reports an error, RenamingSnapshotCommit fails the commit without writing the LATEST hint. The retry in FileStoreCommitImpl then finds the snapshot already committed and returns success, still without writing the hint. If this repeats, LATEST stays behind across many successful commits.

RenamingSnapshotCommit already handles the same situation when the rename returns false. It checks whether the target snapshot file exists with the expected content, treats the commit as done if so, and writes the hint:

boolean committed = fileIO.tryToWriteAtomic(newSnapshotPath, snapshot.toJson());
if (!committed) {
    if (!fileIO.exists(newSnapshotPath)) {
        throw new IOException("Commit snapshot ... failed and ... not found");
    }
    committed = snapshot.equals(Snapshot.fromJson(fileIO.readFileUtf8(newSnapshotPath)));
}
if (committed) {
    snapshotManager.commitLatestHint(snapshot.id());
}

A rename that throws gets none of this. The exception goes straight to FileStoreCommitImpl, which logs Retry commit for exception and retries. Since #5771, the retry finds its own snapshot and returns success. That avoids a duplicate commit, but nothing on that path writes the hint.

We hit this with S3A on an S3-compatible object store. The copy behind the rename succeeded on the server, but the client could not parse the response. Every commit threw and was then reported as successful on retry, and LATEST stayed at the same id for 21 consecutive commits until the hinted snapshot expired. #10323 covers how a stale hint then leaves the table unable to commit, and fixes findLatest so the table recovers. This PR fixes one common way the hint falls behind in the first place.

Change

If tryToWriteAtomic throws but the target snapshot file exists, fall through to the existing "rename returned false" handling: compare the file with the snapshot being committed, and treat the commit as done and write the hint if they match. If the file does not exist, rethrow the original exception as before.

The behaviour stays the same in the other cases:

  • The file exists with different content, for example another writer committed the same id: the commit returns false and is retried, as before.
  • The file cannot be read: the read error is thrown and the commit is retried, as before.

This does not help when writing the hint itself keeps failing. #10323 covers that case.

Tests

  • RenamingSnapshotCommitTest#testCommitSucceedsAndRenameThrows: the rename moves the file and then throws. The commit now succeeds and writes LATEST. Before this change, the commit threw.
  • RenamingSnapshotCommitTest#testCommitRenameThrowsAndTargetMissing: the rename throws without moving the file. The original exception is rethrown.
  • A table-level check, not included in this PR, with a FileIO whose snapshot rename succeeds and then throws. Before this change, LATEST stayed at 1 and the fifth commit failed permanently once snapshot 1 expired. After it, LATEST follows the latest snapshot.
  • All tests under org.apache.paimon.catalog, plus the commit tests in org.apache.paimon.operation: 203 tests pass.
  • Spotless and Checkstyle for paimon-core

RenamingSnapshotCommit already treats a rename that returns false as
committed when the target snapshot file exists with the expected
content, and then writes the LATEST hint. When the rename throws
instead, for example because the response of a successful object store
copy cannot be parsed, the commit fails and the retry in
FileStoreCommitImpl finds the snapshot already committed and returns
success without writing the hint. The LATEST hint then stays behind.

Apply the same check when the rename throws, and rethrow only when the
target snapshot file does not exist.

@Akash3121 Akash3121 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM
Reviewed the ambiguous rename-failure path. Reusing the existing target existence and full snapshot equality checks correctly distinguishes a successful rename with a bad response from a missing or conflicting target, and ensures LATEST is advanced only for the matching snapshot. The focused tests and CI are green. No blocking issues found.

@dev-donghwan

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review, @Akash3121!

If you have time, I'd also appreciate your thoughts on #10323. It comes from the same incident and covers the read side: once the LATEST hint falls behind and the hinted snapshot expires, findLatest keeps returning the expired id and the table can't recover on its own. This PR fixes one way the hint falls behind, and #10323 makes the table recover whatever the cause.

@JingsongLi

Copy link
Copy Markdown
Contributor

+1

@JingsongLi
JingsongLi merged commit 0aa442b into apache:master Oct 2, 2026
18 checks passed
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.

3 participants