hcrosse commented on code in PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#discussion_r4238658422
##########
table/transaction.go:
##########
@@ -3361,13 +3380,17 @@ func (t *Transaction) Commit(ctx context.Context)
(*Table, error) {
withCommitPinnedRefs(t.pinnedRefs),
)
if err != nil {
- // A clean conflict (ErrCommitFailed) committed nothing
and stays
- // retriable. Any other failure leaves the commit state
unknown
- // (the catalog may have accepted it), so mark it
terminal to
- // avoid a double-apply on retry.
+ // A clean conflict (ErrCommitFailed) stays retriable
unless the error
+ // also matches ErrTransactionUnusable: cleanup removed
files the staged
+ // updates reference, or a requirement failed after a
refresh. Any other
+ // failure leaves the commit state unknown, so mark it
terminal to avoid
+ // a double-apply.
if !errors.Is(err, ErrCommitFailed) {
t.committed = true
}
+ if errors.Is(err, ErrTransactionUnusable) {
Review Comment:
With `num-retries=0` a re-commit resends the same stale requirements, so I
made any conflict there terminal. Non-replayable commits too. With retries on,
the next Commit refreshes once and hits the post-refresh check. Since the
default is 0 this changes default behavior, so I wrote up the tradeoff and a
narrower option in the [Retries disabled section of the
description](https://github.com/apache/iceberg-go/pull/2100#issue-5672245891).
Let me know which you'd prefer.
--
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]
