laskoviymishka commented on code in PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#discussion_r4208261469
##########
table/table.go:
##########
@@ -630,15 +631,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)...)
Review Comment:
This fold used to run only after the retry loop. Moving it into the defer
means it now fires on every non-committed exit: a refresh I/O blip, a cancelled
context, a validator rejection on attempt 0. For a rewrite-manifests producer
those paths now delete the staged manifests and wrap `ErrTransactionUnusable`,
so a transient error destroys work the caller could previously re-commit.
That's a behavior change outside this PR's stated scope, and nothing tests
those early-return paths.
I'd settle this deliberately: either keep the fold on the
exhausted-`ErrCommitFailed` and terminal exits only, or add tests pinning the
chosen semantics for a refresh error and a ctx cancel after a rebuild.
`TestCommitAfterFailFastWithoutCleanupStaysRetriable` only covers the
SetProperties case today, which has no superseded source to delete.
##########
table/transaction.go:
##########
@@ -781,14 +796,12 @@ func (t *Transaction) ExpireSnapshots(opts
...ExpireSnapshotsOpt) error {
retainedRefs := make(map[string]SnapshotRef, len(meta.refs))
for refName, ref := range meta.refs {
- // Assert the ref's base snapshot id so we don't accidentally
- // expire snapshots that are now referenced by concurrently
- // updated refs. Refs the transaction itself staged (absent on
- // the base) get no assertion here: the update that created
- // them carries its own base-state requirement.
+ // Pin refs to avoid expiring snapshots retained by a
concurrent ref update.
+ // Refs staged by this transaction already have absence
assertions.
if id := t.baseRefSnapshotID(refName); id != nil {
reqs = append(reqs, AssertRefSnapshotID(refName, id))
}
+ pinned = append(pinned, refName)
Review Comment:
Pin a ref only when the expire actually removes something. Appending every
ref to `pinned` here unconditionally is the round-2 regression, still present:
on a no-op expire `applyPinned` publishes a pin on every ref including the
commit branch, so an `Append` plus a non-expiring `ExpireSnapshots` in one
transaction loses refresh-and-replay. A peer's non-conflicting append then
fails the first refresh with `ErrCommitFailed` and the retry budget never gets
used. Standalone it's harmless since `Commit` short-circuits on empty updates,
but it bites as soon as the transaction carries other work.
I'd pass `pinned` to `applyPinned` only when `len(snapsToDelete) > 0` (or a
`RemoveSnapshotRef` was staged), and `nil` otherwise, then add a test: append
on `main`, an expire that removes nothing, a peer append in between, asserting
the commit still succeeds on retry. If pinning every ref is intended, let's say
so in the comment.
##########
table/table.go:
##########
@@ -50,14 +50,17 @@ 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:
`ErrCommitFailed` now spans two cases a caller has to treat differently: a
retry-safe head conflict, and a non-rebasable failure (stale
schema/spec/sort-order, or a staging-time `Validate` in `applyLocked`) where
re-committing the same `Transaction` only reproduces the identical error. The
sentinel text still reads "refresh and try again" and `Commit` leaves the
transaction re-committable on any `ErrCommitFailed`, so an external retry loop
keyed on `errors.Is(err, ErrCommitFailed)` will spin on the non-rebasable case.
The godoc here does call it out, which helps. But I'd give callers something
to branch on without string-matching: either a distinct sentinel for "rebuild
required," or mark the transaction non-re-committable after a non-rebased
failure the same way the cleanup path already does with
`ErrTransactionUnusable`. Not blocking the way the expire one is, but this is
the public retry contract, so worth settling now.
--
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]
