hcrosse commented on code in PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#discussion_r4259590786
##########
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:
Agreed, the string surgery is fragile. I see two clean options and I'm not
sure which you'd prefer:
1. Wrap the validator error with plain `%w` and drop
`validationConflictError`. The message keeps all the detail but reads
"transaction cannot be committed again: validation conflict: commit failed,
refresh and try again: concurrent data files added: …".
2. Give the three conflict sentinels in `conflict_validation.go` their own
text and have them match `ErrCommitFailed` through an `Is` method, so the
message reads "…: validation conflict: concurrent data files added: …". That
changes those sentinels' `Error()` text on main, though `errors.Is` behaves the
same.
Which would you rather have?
--
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]
