hcrosse commented on code in PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#discussion_r4238658346
##########
table/table.go:
##########
@@ -690,11 +704,9 @@ func (t Table) doCommit(ctx context.Context, updates
[]Update, reqs []Requiremen
current = fresh.metadata
reqs = rewriteRefSnapshotRequirements(reqs, co.branch,
current, co.pinnedRefs)
- // A pinned assertion the fresh catalog state violates
can
- // never succeed — fail now instead of burning the
remaining
- // retries on it.
- if err := validatePinnedRefRequirements(reqs,
co.pinnedRefs, current); err != nil {
- return nil, fmt.Errorf("%w: explicit ref
requirement failed: %w", ErrCommitFailed, err)
+ if err := validateNonRebasedRequirements(reqs,
co.branch, co.pinnedRefs, current); err != nil {
+ return nil, fmt.Errorf("%w: requirement no
longer holds after refresh: %w: %w",
Review Comment:
Good catch. It now starts with "transaction cannot be committed again" and
still matches ErrCommitFailed. I also dropped the second wrap in the defer.
##########
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.
##########
table/table.go:
##########
@@ -630,15 +632,27 @@ func (t Table) doCommit(ctx context.Context, updates
[]Update, reqs []Requiremen
// CommitTable, where the catalog may have silently accepted the commit
and one
// of the "orphaned" files may actually be the live snapshot.
cleanupOrphans := true
+ committed := false
defer func() {
- if !cleanupOrphans || len(orphanedManifests) == 0 {
+ if !cleanupOrphans {
return
}
+ // The final attempt's manifests are orphaned only if the
commit failed.
+ for _, u := range updates {
+ if su, ok := u.(*addSnapshotUpdate); ok &&
su.supersededSource != nil {
+ orphanedManifests = append(orphanedManifests,
su.supersededSource.supersededManifests(committed)...)
+ }
+ }
for _, path := range orphanedManifests {
if removeErr := wfs.Remove(path); removeErr != nil {
log.Printf("Warning: failed to delete orphaned
manifest list %s: %v", path, removeErr)
}
}
+ // Resubmitting staged updates after cleanup would reference
deleted files.
+ if !committed && retErr != nil && len(orphanedManifests) > 0 {
Review Comment:
Yes, that's intended. I spelled it out in the Commit comment and
concurrent-writes.md. The wrap now keys on a flag set where files are removed.
##########
table/table.go:
##########
@@ -50,14 +50,18 @@ import (
// commit fails due to a concurrent modification (e.g. HTTP 409 Conflict
// from the REST catalog). Catalog implementations should wrap this
// error so that callers using errors.Is(err, table.ErrCommitFailed)
-// can detect retryable commit conflicts.
-//
-// Currently only catalog/rest wraps this sentinel; Glue, SQL, and Hive
-// catalogs return their conflict errors raw and will not trigger
-// retries until follow-up work wires them through (tracked under
-// issue #830).
+// can detect retryable commit conflicts. Failed requirements also wrap it.
Review Comment:
Done, I reworded the opener.
--
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]
