zeroshade commented on PR #2100:
URL: https://github.com/apache/iceberg-go/pull/2100#issuecomment-6369413577
Re-review after the merge from main. My earlier change request (no-op
`ExpireSnapshots` pinning the branch) is resolved, thanks.
The merge picked up #2248, which conflicts with this PR.
`catalog/internal.ValidateRequirements` now wraps every requirement failure as
`fmt.Errorf("%w: %w", table.ErrCommitFailed, err)`. So on SQL, Hive, Glue and
Hadoop, the two failures this PR keeps terminal ("tags cannot be transaction
targets", "last assigned partition id is missing") match `ErrCommitFailed`
again, and an external `errors.Is` loop would spin on them. It also prefixes
every message with "commit failed, refresh and try again: ". Since
`requirementError` now matches `ErrCommitFailed` on its own,
`ValidateRequirements` can return `err` unchanged apart from the
`ErrNoSuchTable` case. Could you make that change and add a row to
`TestValidateRequirementsClassifiesFailures` asserting that the terminal case
does not match `ErrCommitFailed`?
On the open question with @laskoviymishka: for what it's worth, I'm fine
with errors matching both sentinels if it's documented as the rule for every
terminal case and the bounded external-loop test lands. For the message, I'd
prefer option 1 (plain `%w`, drop `validationConflictError`) over changing the
sentinels' text. I'll defer to @laskoviymishka on both.
--
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]
