hcrosse commented on code in PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#discussion_r4259587974
##########
table/table.go:
##########
@@ -684,39 +732,62 @@ func (t Table) doCommit(ctx context.Context, updates
[]Update, reqs []Requiremen
}
if t.metadata != nil && fresh.metadata != nil {
if err := validateTableUUID(t.identifier,
t.metadata.TableUUID(), fresh.metadata.TableUUID()); err != nil {
- return nil, err
+ return nil, fmt.Errorf("%w: table was
replaced: %w", ErrTransactionUnusable, err)
}
}
current = fresh.metadata
- reqs = rewriteRefSnapshotRequirements(reqs, co.branch,
current, co.pinnedRefs)
+ reqs = rewriteRefSnapshotRequirements(reqs, co.branch,
current, pinned)
- // 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, pinned, current); err != nil {
+ if co.noReplay {
+ return nil, fmt.Errorf("%w: requirement
no longer holds after refresh (%s): %w",
+ ErrTransactionUnusable,
noReplayHint, err)
+ }
+
+ return nil, fmt.Errorf("%w: requirement no
longer holds after refresh: %w", ErrTransactionUnusable, err)
Review Comment:
I'd prefer to keep the current matching and document it, with each sentinel
answering a different question:
- `ErrCommitFailed` means a concurrent change caused the failure. A new
transaction built from a reloaded table may succeed.
- `ErrTransactionUnusable` means this transaction can't be committed again.
I checked the external loop against each terminal case. For stale
requirements, validator conflicts and deleted branches, rebuilding from the
reloaded state succeeds, so matching `ErrCommitFailed` lets the loop recover.
Tag targets, replaced tables and diverged bases don't match `ErrCommitFailed`,
so the loop stops.
Removing `ErrCommitFailed` from terminal errors would mean either keeping
only the cause's text, which breaks `errors.Is(err, ErrConflictingDataFiles)`,
or a wrapper that hides `ErrCommitFailed` even though `ErrConflictingDataFiles`
wraps it. I'd add the bounded external-loop test you asked for to show both
outcomes.
Does that work for you, or would you still rather terminal errors never
match `ErrCommitFailed`?
--
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]
