[core] Treat a snapshot rename that throws but succeeded as committed - #10324
Conversation
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
left a comment
There was a problem hiding this comment.
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.
|
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 |
|
+1 |
Related: #10323 makes the table recover from a stale
LATESThint, whatever made it stale.Purpose
When a snapshot rename actually succeeds but the file system reports an error,
RenamingSnapshotCommitfails the commit without writing theLATESThint. The retry inFileStoreCommitImplthen finds the snapshot already committed and returns success, still without writing the hint. If this repeats,LATESTstays behind across many successful commits.RenamingSnapshotCommitalready handles the same situation when the rename returnsfalse. It checks whether the target snapshot file exists with the expected content, treats the commit as done if so, and writes the hint:A rename that throws gets none of this. The exception goes straight to
FileStoreCommitImpl, which logsRetry commit for exceptionand 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
LATESTstayed 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 fixesfindLatestso the table recovers. This PR fixes one common way the hint falls behind in the first place.Change
If
tryToWriteAtomicthrows 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:
falseand 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 writesLATEST. Before this change, the commit threw.RenamingSnapshotCommitTest#testCommitRenameThrowsAndTargetMissing: the rename throws without moving the file. The original exception is rethrown.FileIOwhose snapshot rename succeeds and then throws. Before this change,LATESTstayed at 1 and the fifth commit failed permanently once snapshot 1 expired. After it,LATESTfollows the latest snapshot.org.apache.paimon.catalog, plus the commit tests inorg.apache.paimon.operation: 203 tests pass.paimon-core