hcrosse commented on PR #2100: URL: https://github.com/apache/iceberg-go/pull/2100#issuecomment-6111870227
Thanks for sticking with this. The last few rounds kept turning up new edge cases, and they all came from one thing: calling `Commit` again resent the original stale requirements and updates. So instead of predicting whether that could land, I changed what a re-commit does. A re-commit is now the next retry. After a failed `Commit` that committed nothing, the next `Commit` refreshes, rebases the branch head assertion, rebuilds and runs validators before submitting. I think that's your refresh plus `validateNonRebasedRequirements` suggestion, run when `Commit` is called again, so the branch head gets rebased instead of resent stale. It's the same path as an in-call retry, similar to how Java's [`BaseTransaction.commitTransaction`](https://github.com/apache/iceberg/blob/140f4654bfcc90d3a728b74cff9f2fa8f92182d9/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L250) re-applies pending updates on each attempt. To make that safe, a failed commit only deletes files it wrote. The staged files stay until a commit lands, and an abandoned transaction leaves them for orphan file removal, like main already does for an append with retries off. This reverses what I said last round about those exits being terminal. That was true on main, but deleting the staged files was the only thing making them terminal. `ErrTransactionUnusable` now only means a refresh showed the commit can't land. | Case | Next `Commit` | |---|---| | Transient conflict (lock, CAS race) | lands | | Branch head moved | rebases and lands | | Stale schema, spec, sort order or pinned ref | unusable, nothing submitted | | Validator conflict, diverged base, removed rewrite input | unusable | | Branch deleted or turned into a tag, table replaced | unusable | | Non-replayable commit | lands if the head didn't move, otherwise unusable | | Refresh error or cancelled context | resumes | | Unknown commit state | terminal, nothing deleted | | `TableCommit` after a failed `Commit` | refused, use `Commit` to resume | On your asks: - Transient conflicts stay retriable: `TestTransactionCommit_RetriableAfterCleanConflict`, `TestConflictClearedBeforeRecommitLands` and `TestTransactionCommit_BusyTableWithRetriesDisabledResumesUntilItLands`. - The `conflict` field is gone, so `ErrCommitFailed` only matches when the cause is a real conflict. - No terminal string says "refresh and try again". `TestResumeDeterministicRejectionsMakeTransactionUnusable` checks every terminal case. - The no-op `ExpireSnapshots` checks usable now. - Pinning the commit branch on a real expire is deliberate, and I documented it on `ExpireSnapshots` and in `concurrent-writes.md`. I left a few things for follow-ups: raising the default `num-retries`, SQL treating builder rejections as an unknown commit state, and cleaning up merged manifests left by abandoned rewrites. A commit that lands first try does no extra I/O. A re-commit costs one `LoadTable` plus a manifest-list rebuild. -- 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]
