hcrosse commented on code in PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#discussion_r4390162452
##########
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:
Done. I rewrote the `ErrCommitFailed` doc around the two questions and
listed which cases match. I also added
`TestRetryLoopOnCommitFailedWithNewTransactions` on SQLite. A validator
conflict and a stale schema commit on the second pass, and a diverged base
stops the loop after the first.
##########
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:
I added a `matchesCommitFailed` field so every row asserts its side, and the
tag-target, replaced-table and diverged-base rows now check `NotErrorIs(err,
ErrCommitFailed)`.
##########
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:
I switched to plain `%w` and dropped `validationConflictError`. A per-row
`mentionsRetry` flag now skips the "refresh and try again" check for the
validator rows.
--
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]
