laskoviymishka commented on code in PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#discussion_r4232185269
##########
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:
This is the cleanup-on-early-return semantics I flagged last round, and the
new `TestTransactionCommit_UnusableAfterTransientFailureFollowingRebuild` reads
like you've settled on it being intended, which is fine, I just want it to be
on purpose. A transient refresh error or a ctx cancel after a rebuilt attempt
now deletes a rewrite producer's staged manifests and marks the transaction
unusable, where before it stayed re-committable. If that's the contract, the
wording should say so plainly: the `Commit` comment and `concurrent-writes.md`
only mention "cleanup removed files" and "requirement failed after refresh",
but this also fires on an ordinary transient error once a rebuild has happened.
One thing nagging at me about the signal: the wrap keys on
`len(orphanedManifests) > 0`, which is a side effect of whether a
producer/rebuild ran, not whether staged updates actually lost files. It's
correct today, but an attempt-0 failure with no rebuild wraps nothing while a
rewrite producer in the same spot wraps, so a future change to what counts as
an orphan would silently flip reusability. An explicit "staged files were
deleted" bool set at the delete site would make the invariant hold regardless.
wdyt?
##########
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:
Small one while we're in here: the opening still says `ErrCommitFailed` lets
callers "detect retryable commit conflicts", but four lines down the same
comment says an error matching both sentinels can't succeed on retry. I'd
soften the opener to something like "retryable unless it also matches
`ErrTransactionUnusable`" so the two halves don't read against each other.
##########
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:
This is the conflation thread from last round, and the text is where it
bites hardest now. `ErrCommitFailed.Error()` is "commit failed, refresh and try
again", so this composes to "commit failed, refresh and try again: requirement
no longer holds after refresh: transaction cannot be committed again", where
the first and last clauses contradict and a log reader is told to retry a
terminal error. I'd keep both sentinels for `errors.Is` but control the string:
wrap the cause and the two sentinels with `%w` without `ErrCommitFailed`'s text
leading, so "cannot be committed again" isn't buried behind "try again".
The defer above also re-wraps `ErrTransactionUnusable` when
`orphanedManifests` is non-empty on this same path, so we can get the sentinel
and the "build a new transaction" phrase twice. Skipping the re-wrap when
`errors.Is(retErr, ErrTransactionUnusable)` already holds cleans that up.
##########
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:
Still the conflation thread: this only flips `unusable` when the error
already carries `ErrTransactionUnusable`, and two paths exit without it. With
`num-retries=0` there's no refresh, so `validateNonRebasedRequirements` never
runs; and on the final attempt the catalog's `CommitTable` rejects the
non-rebased requirement with a plain `ErrCommitFailed`. Both exit the loop with
`ErrCommitFailed` and no `ErrTransactionUnusable`, so `t.unusable` stays false
and the transaction is still re-committable. Re-committing runs the same stale
requirement against the same state and fails identically, which is exactly the
external `errors.Is(err, ErrCommitFailed)` loop spinning on a hopeless commit.
This PR newly routes SQL/Glue/Hive requirement conflicts through
`ErrCommitFailed`, so that spin is now reachable on the local catalogs, not
just REST.
I'd check the failed requirement against freshly loaded state on the last
attempt too, or mark the transaction unusable when a non-rebasable requirement
is what failed, so a re-commit short-circuits instead of replaying.
`TestStaleAppendRetriesRequirementFailure` already asserts the
plain-`ErrCommitFailed` outcome for `num-retries=0`, so it'd be worth extending
it to assert the second `Commit` doesn't just repeat.
--
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]
