zeroshade commented on code in PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#discussion_r4168435841
##########
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:
**Blocking.** This used to be terminal on SQL, Glue, Hive and Hadoop. Now
that it's retryable, `doCommit` calls `rewriteRefSnapshotRequirements` to point
this assertion at the refreshed head and resubmits the same updates. That
breaks the guards in `RollbackToSnapshot` and `ExpireSnapshots`, which rely on
this check failing when the branch has moved. A SQL probe with `num-retries=2`:
- A stale `RollbackToSnapshot(s1)` after a peer append now commits, and
`main` ends up as `[s1]`. The peer's append is gone.
- A stale `ExpireSnapshots(WithRetainLast(3))` after a peer rollback removes
snapshots that are back inside the retain window.
Please pin the guarded branch in both operations (via `pinnedRefs`, as
`Transaction.AssertRefSnapshotID` does) so the retry fails fast. Java's
`RemoveSnapshots` recomputes from `ops.refresh()` instead.
##########
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:
Minor: the "Catalog support" note in `website/src/concurrent-writes.md`
still says Glue, SQL and Hive conflicts don't trigger retries, and it doesn't
mention Hadoop. Please update it along with this doc.
##########
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:
Minor: only the branch's snapshot-id assertion is rewritten between
attempts. Stale schema, spec, sort-order, UUID or other-ref assertions fail the
same way on every retry, costing a backoff and a `LoadTable` each time. After
the refresh in `doCommit`, check those requirements against `current` and
return immediately. Java avoids the retry through `ValidationFailureException`.
--
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]
