hcrosse commented on code in PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#discussion_r4174166328
##########
table/requirements.go:
##########
@@ -318,14 +330,14 @@ func (a *assertRefSnapshotID) Validate(meta Metadata)
error {
}
if a.SnapshotID == nil {
- return fmt.Errorf("requirement failed: %s %q was
created concurrently", r.SnapshotRefType, a.Ref)
+ return requirementFailed("requirement failed: %s %q was
created concurrently", r.SnapshotRefType, a.Ref)
}
if r.SnapshotID != *a.SnapshotID {
- return fmt.Errorf("requirement failed: %s %q has
changed: expected id %d, found %d", r.SnapshotRefType, a.Ref, *a.SnapshotID,
r.SnapshotID)
+ return requirementFailed("requirement failed: %s %q has
changed: expected id %d, found %d", r.SnapshotRefType, a.Ref, *a.SnapshotID,
r.SnapshotID)
Review Comment:
Hey Matt('s agent), thanks for the review!
I pinned the refs guarded by `RollbackToSnapshot` and `ExpireSnapshots`
using the same `pinnedRefs` mechanism as `Transaction.AssertRefSnapshotID`,
including branches staged by the transaction, so stale retries fail with
`ErrCommitFailed` instead of rebasing. The pins and updates are now published
together under one lock. I added SQL catalog tests for both of your probes with
two retries enabled, plus staged-branch variants.
All of these changes made the surface-area of this PR kind of large, but I
think make it a better change overall.
##########
table/requirements.go:
##########
@@ -227,6 +227,18 @@ func (b baseRequirement) GetType() string {
return b.Type
}
+// requirementFailed returns an error that matches ErrCommitFailed, so the
+// commit can be retried against refreshed metadata.
Review Comment:
I changed `doCommit` to validate every requirement it doesn't rebase after
refresh and return `ErrCommitFailed` immediately if one fails. I also made
these early exits clean up rewrite manifests. When cleanup removes staged
files, the transaction returns `ErrTransactionUnusable` rather than
resubmitting, including through `TableCommit` and multi-table transactions.
Failures that clean up nothing remain retriable.
##########
table/table.go:
##########
@@ -50,12 +50,8 @@ 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. Requirement validation failures,
+// such as a branch that has moved since the table was loaded, also wrap it.
Review Comment:
I updated the catalog support note to cover all built-in catalogs, including
Hadoop, and clarified the `ErrCommitFailed` doc about requirements that aren't
rebased. I also corrected the API docs to say retries must be enabled rather
than claiming `Commit` retries automatically.
--
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]
