hcrosse commented on code in PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#discussion_r4242564413
##########
table/table.go:
##########
@@ -778,24 +795,18 @@ func (t Table) doCommit(ctx context.Context, updates
[]Update, reqs []Requiremen
// of replaying (see commitOpts.noReplay). The annotation
// distinguishes this abort from an exhausted retry budget while
// preserving errors.Is(err, ErrCommitFailed).
+ // Transaction.Commit resends the same requirements and
removals, so
+ // without a retry to rebase them a re-commit can only fail
again.
if co.noReplay {
- return nil, fmt.Errorf("%w (commit carries
snapshot-relative delete-file removals and cannot be replayed; reload the table
and rebuild the removals)", err)
+ return nil, &unusableCommitError{
+ reason: "commit carries snapshot-relative
delete-file removals and cannot be replayed; reload the table and rebuild the
removals",
+ cause: err,
+ conflict: true,
+ }
}
- }
-
- // Inner data manifests written by superseded retry attempts (a rewrite
- // re-merges everything on each retry) are orphaned objects. On a safe
- // exhausted-ErrCommitFailed failure (err != nil here) nothing
committed, so
- // the accumulator also folds in the final attempt's manifests. On
success
- // the committed snapshot references those, so they are excluded. The
defer
- // skips cleanup only on the unsafe non-ErrCommitFailed path, which
returned
- // above with cleanupOrphans = false.
- for _, u := range updates {
- su, ok := u.(*addSnapshotUpdate)
- if !ok || su.supersededSource == nil {
- continue
+ if cfg.numRetries == 0 {
Review Comment:
I reworked this. A re-commit now resumes as the next retry, so a transient
conflict lands and only a commit that a refresh shows can't land becomes
unusable. Details in [this
comment](https://github.com/apache/iceberg-go/pull/2100#issuecomment-6111870227).
##########
table/table.go:
##########
@@ -888,31 +897,46 @@ func rewriteRefSnapshotRequirements(reqs []Requirement,
branch string, fresh Met
return out
}
-// validatePinnedRefRequirements validates every assert-ref-snapshot-id
-// requirement on a pinned branch against the freshly refreshed catalog
-// metadata. A failure means the branch has changed from the required
-// snapshot: the assertion is never rewritten, so it can never hold and
-// the commit must fail instead of retrying.
-func validatePinnedRefRequirements(reqs []Requirement, pinned
map[string]struct{}, fresh Metadata) error {
- if len(pinned) == 0 {
- return nil
- }
+// validateNonRebasedRequirements checks the requirements a retry does not
+// rebase. Only unpinned commit-branch assertions are rebased, so any other
+// requirement that fails against refreshed metadata cannot succeed on retry.
+func validateNonRebasedRequirements(reqs []Requirement, branch string, pinned
map[string]struct{}, fresh Metadata) error {
for _, r := range reqs {
- a, ok := r.(*assertRefSnapshotID)
- if !ok {
- continue
- }
- if _, isPinned := pinned[a.Ref]; !isPinned {
- continue
+ if a, ok := r.(*assertRefSnapshotID); ok && a.Ref == branch {
+ if _, isPinned := pinned[a.Ref]; !isPinned {
+ continue
+ }
}
- if err := a.Validate(fresh); err != nil {
+ if err := r.Validate(fresh); err != nil {
return err
}
}
return nil
}
+// unusableCommitError ends a commit that no re-commit of the same updates
+// can fix. It matches ErrTransactionUnusable and the cause, and also
+// ErrCommitFailed when conflict is set, but its message leads with
+// ErrTransactionUnusable so it does not read as retryable.
+type unusableCommitError struct {
+ reason string
+ cause error
+ conflict bool
+}
+
+func (e *unusableCommitError) Error() string {
+ return fmt.Sprintf("%s: %s: %s", ErrTransactionUnusable, e.reason,
e.cause)
Review Comment:
None of the terminal strings say "refresh and try again" now, and a test
checks every terminal case.
##########
table/table.go:
##########
@@ -888,31 +897,46 @@ func rewriteRefSnapshotRequirements(reqs []Requirement,
branch string, fresh Met
return out
}
-// validatePinnedRefRequirements validates every assert-ref-snapshot-id
-// requirement on a pinned branch against the freshly refreshed catalog
-// metadata. A failure means the branch has changed from the required
-// snapshot: the assertion is never rewritten, so it can never hold and
-// the commit must fail instead of retrying.
-func validatePinnedRefRequirements(reqs []Requirement, pinned
map[string]struct{}, fresh Metadata) error {
- if len(pinned) == 0 {
- return nil
- }
+// validateNonRebasedRequirements checks the requirements a retry does not
+// rebase. Only unpinned commit-branch assertions are rebased, so any other
+// requirement that fails against refreshed metadata cannot succeed on retry.
+func validateNonRebasedRequirements(reqs []Requirement, branch string, pinned
map[string]struct{}, fresh Metadata) error {
for _, r := range reqs {
- a, ok := r.(*assertRefSnapshotID)
- if !ok {
- continue
- }
- if _, isPinned := pinned[a.Ref]; !isPinned {
- continue
+ if a, ok := r.(*assertRefSnapshotID); ok && a.Ref == branch {
+ if _, isPinned := pinned[a.Ref]; !isPinned {
+ continue
+ }
}
- if err := a.Validate(fresh); err != nil {
+ if err := r.Validate(fresh); err != nil {
return err
}
}
return nil
}
+// unusableCommitError ends a commit that no re-commit of the same updates
+// can fix. It matches ErrTransactionUnusable and the cause, and also
+// ErrCommitFailed when conflict is set, but its message leads with
+// ErrTransactionUnusable so it does not read as retryable.
+type unusableCommitError struct {
+ reason string
+ cause error
+ conflict bool
+}
+
+func (e *unusableCommitError) Error() string {
+ return fmt.Sprintf("%s: %s: %s", ErrTransactionUnusable, e.reason,
e.cause)
+}
+
+func (e *unusableCommitError) Unwrap() []error {
+ if e.conflict {
Review Comment:
I dropped it. `Unwrap` is just `ErrTransactionUnusable` plus the cause, so
`ErrCommitFailed` only matches when the cause is a real conflict.
##########
table/transaction.go:
##########
@@ -867,7 +880,13 @@ func (t *Transaction) ExpireSnapshots(opts
...ExpireSnapshotsOpt) error {
updates = append(updates,
NewRemoveSnapshotsUpdate(snapsToDelete, cfg.postCommit))
}
- return t.apply(updates, reqs)
+ // A no-op expiry must not pin the commit branch, which would stop the
+ // transaction's other updates from rebasing on retry.
+ if len(updates) == 0 {
Review Comment:
Good catch, it checks usable 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]
