laskoviymishka commented on code in PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#discussion_r4242916943
##########
table/table.go:
##########
@@ -888,31 +951,66 @@ 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
}
+const noReplayHint = "commit carries snapshot-relative delete-file removals
and cannot be replayed; reload the table and rebuild the removals"
+
+// commitStateUnknownError marks a CommitTable failure after which the catalog
+// may have accepted the commit. Every other doCommit failure committed
+// nothing.
+type commitStateUnknownError struct{ err error }
+
+func (e *commitStateUnknownError) Error() string { return e.err.Error() }
+
+func (e *commitStateUnknownError) Unwrap() error { return e.err }
+
+// validationConflictError ends a commit a validator rejected after a
+// refresh because of what a concurrent commit changed. Validators report
+// such a rejection with an error that matches ErrCommitFailed, and the same
+// concurrent commit rejects every later attempt. It matches
+// ErrTransactionUnusable and the validator's error. Its message keeps the
+// validator's detail without ErrCommitFailed's advice to try again.
+type validationConflictError struct{ err error }
+
+func (e *validationConflictError) Error() string {
+ detail, ok := strings.CutPrefix(e.err.Error(),
ErrCommitFailed.Error()+": ")
Review Comment:
`CutPrefix` only strips the sentinel when the validator wrote it as an exact
leading prefix. A validator that wraps it any other way, say
`fmt.Errorf("custom check: %w", ErrCommitFailed)`, falls through to the generic
`a concurrent commit conflicts with the staged changes` and silently drops its
real detail. I'd build this message from the validator's own error (a typed
conflict carrying the detail) rather than string surgery on the formatted text,
which also breaks the moment the sentinel wording changes.
##########
table/commit_refresh_replay_test.go:
##########
@@ -215,6 +220,288 @@ func TestDoCommit_ValidatorRejectsOnRefresh(t *testing.T)
{
"validator rejection on retry must abort before re-issuing
CommitTable")
}
+// Why: after a refresh, a rejection that does not depend on timing fails
+// every later attempt and every re-commit the same way, so the transaction
+// must become unusable rather than leave an errors.Is(ErrCommitFailed) loop
Review Comment:
This test's comment says the transaction `must become unusable rather than
leave an errors.Is(ErrCommitFailed) loop spinning`, but several rows (stale
schema, commit branch deleted, non-replayable commit whose branch moved) assert
`ErrorIs(err, ErrCommitFailed)` on the unusable error, so it pins the dual
match rather than preventing it. Whatever we land on in the comment above, this
test should assert it on every unusable row instead of row-by-row. While we're
here, nothing exercises the external `errors.Is(ErrCommitFailed)` retry loop
this whole rework exists to stop, so I'd add one with a bounded call count.
##########
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:
Same ErrCommitFailed/ErrTransactionUnusable conflation we've been circling
since round 2, and dropping the hard-coded `conflict: true` was the right move,
but it didn't actually resolve: the cause here is a `requirementError` whose
`Unwrap` returns `ErrCommitFailed`, so this terminal error still matches
`errors.Is(err, ErrCommitFailed)`. `validationConflictError.Unwrap` (line 1000)
does the same, while the tag, replaced-table, and diverged wraps match only
`ErrTransactionUnusable`. So which sentinels a terminal error carries depends
on which helper built the cause, not on a rule.
I'd pick one before merge: either strip `ErrCommitFailed` off the cause when
wrapping as unusable (expose the detail through a typed error, not the
sentinel), so `errors.Is(ErrCommitFailed)` keeps meaning retry-safe like it
does everywhere else in this PR; or make the dual match a deliberate,
documented contract that holds for all five wraps and have callers test
`ErrTransactionUnusable` first. Right now it's incidental, and an external
`errors.Is(ErrCommitFailed)` loop that rebuilds the transaction each pass never
sees the latch.
--
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]
