fix(transaction): move flag assignment after auto-commit - #426
shangxinli wants to merge 2 commits into
Conversation
Set last_update_committed_ before calling Commit(), but revert the flag if auto-commit fails. This ensures: 1. Commit() can proceed (it requires last_update_committed_ == true) 2. If commit fails, the flag is reverted to false 3. Transaction state remains consistent on failure
9cc0948 to
17c03f1
Compare
| update->ApplyTo(*metadata_builder_); | ||
| } | ||
|
|
||
| last_update_committed_ = true; |
There was a problem hiding this comment.
Perhaps it is simpler to move line 68 to be after the if (auto_commit_) block?
There was a problem hiding this comment.
make sense! it makes simpler.
There was a problem hiding this comment.
It seems the test failed after we move this line. The problem is Commit() requires this flag to be true at the start. So we need to set it to be true before that.
There was a problem hiding this comment.
I think the name last_update_committed_ might confuse readers a little bit because it only means whether previous created pending update has called commit to apply changes to the transaction. We use this flag to make sure updates are created and committed sequentially to make the state of transaction simpler. Since it has nothing to do with the transaction object itself, I'm hesitant to set last_update_committed_ to false when transaction.Commit fails. Perhaps we should rename it to last_update_applied or something more precise. WDYT?
There was a problem hiding this comment.
Yeah, it seem in java the flag hasLastOpCommitted (the equivalent flag) tracks whether PendingUpdate.commit() was called, not whether Transaction.commitTransaction()
succeeded.
5afbffd to
7c38f49
Compare
Match Java's BaseTransaction where hasLastOpCommitted tracks whether PendingUpdate.commit() was called, not Transaction.commitTransaction(). Key changes: - Remove last_update_committed_ reversion on Transaction::Commit() failure - The flag tracks PendingUpdate state (updates applied to metadata_builder_) - Transaction persistence to catalog is a separate concern This aligns with Java BaseTransaction.TransactionTableOperations.commit() at line 645 which sets hasLastOpCommitted=true and never reverts it based on transaction commit results.
7c38f49 to
fb231c4
Compare
2 participants
Move last_update_committed_ assignment to after auto-commit attempt.
If Commit() fails, ICEBERG_RETURN_UNEXPECTED returns early and the flag remains false, preventing the transaction from incorrectly reporting the update as committed when it actually failed.