zeroshade commented on code in PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#discussion_r4198578149
##########
table/transaction.go:
##########
@@ -3368,6 +3381,9 @@ func (t *Transaction) Commit(ctx context.Context)
(*Table, error) {
if !errors.Is(err, ErrCommitFailed) {
t.committed = true
}
+ if errors.Is(err, ErrTransactionUnusable) {
+ t.unusable = true
+ }
Review Comment:
The comment above this block still says a clean conflict `stays retriable`,
which is no longer true once cleanup ran. That now covers ordinary appends too:
the first rebuild orphans the staged manifest list, the defer deletes it, and
the failed commit makes the transaction unusable. A throwaway test with
`AddDataFiles`, `num-retries=3` and four `ErrCommitFailed` responses gets
`ErrCommitFailed` + `ErrTransactionUnusable` from the first `Commit` and
`ErrTransactionUnusable` from the second. I think that's the right outcome,
since resubmitting would reference the deleted manifest list. But the only
`ErrTransactionUnusable` test goes through `RewriteManifests`, and
`TestTransactionCommit_RetriableAfterExhaustedInternalRetries` only covers a
properties-only transaction. Please update the comment and add a
transaction-level test for the append path.
##########
table/transaction.go:
##########
@@ -867,7 +880,7 @@ func (t *Transaction) ExpireSnapshots(opts
...ExpireSnapshotsOpt) error {
updates = append(updates,
NewRemoveSnapshotsUpdate(snapsToDelete, cfg.postCommit))
}
- return t.apply(updates, reqs)
+ return t.applyPinned(updates, reqs, pinned)
Review Comment:
Blocking: when the expire stages nothing (no snapshots or refs to remove),
this still pins every ref, the commit branch included. A transaction that
writes and also runs a no-op `ExpireSnapshots` can no longer rebase onto a
peer's append. I checked this against a REST-style catalog whose `main` moved
from 100 to 200, with `num-retries=2`. `SetProperties` alone commits on the
second attempt. `SetProperties` plus a no-op `ExpireSnapshots()` also committed
before this PR, but now fails after one attempt with `requirement no longer
holds after refresh`. A no-op expire has nothing to guard, so I'd skip its
requirements and pins. The `table`, `catalog/sql` and `cmd/iceberg` tests still
pass with this change:
```suggestion
if len(updates) == 0 {
return nil
}
return t.applyPinned(updates, reqs, pinned)
```
--
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]
