zeroshade commented on code in PR #2350:
URL: https://github.com/apache/iceberg-go/pull/2350#discussion_r4415025722
##########
table/table.go:
##########
@@ -62,6 +62,15 @@ import (
// stops.
var ErrCommitFailed = errors.New("commit failed, refresh and try again")
+// ErrCommitRejected means the catalog rejected a commit without writing
+// anything. It is the Go equivalent of Java's CleanableFailure.
+//
+// Transaction.Commit does not retry it, but a later Commit can try again.
+// Staged files are retained. Files created during retries are deleted unless
+// the transaction can reuse them. Errors that also match ErrCommitFailed
+// are treated as conflicts.
+var ErrCommitRejected = errors.New("commit rejected")
Review Comment:
Non-blocking: consider noting here that the REST catalog treats every 400 as
a rejection, including one whose error type is `IllegalArgumentException`. That
differs from Java's `CleanableFailure`. The PR description explains why, but it
would be better recorded in the code.
##########
catalog/rest/rest.go:
##########
@@ -114,6 +114,12 @@ var (
ErrCommitStateUnknown = fmt.Errorf("%w: commit failed due to unknown
reason", ErrRESTError)
ErrOAuthError = fmt.Errorf("%w: oauth error", ErrRESTError)
+ // These statuses mean the catalog did not change the table.
+ errCommitBadRequest = fmt.Errorf("%w: %w", ErrBadRequest,
table.ErrCommitRejected)
+ errCommitUnauthorized = fmt.Errorf("%w: %w", ErrUnauthorized,
table.ErrCommitRejected)
+ errCommitForbidden = fmt.Errorf("%w: %w", ErrForbidden,
table.ErrCommitRejected)
+ errCommitNoSuchTable = fmt.Errorf("%w: %w", catalog.ErrNoSuchTable,
table.ErrCommitRejected)
Review Comment:
Non-blocking: a commit that gets a 404 used to leave the transaction
permanently failed, and now it stays resumable (a recommit then fails when it
reloads the table). A one-line note in `website/src/releases.md` would help
users notice the change.
##########
catalog/internal/utils.go:
##########
@@ -281,7 +281,22 @@ func ValidateRequirements(ident table.Identifier, current
table.Metadata, reqs [
return nil
}
-func UpdateAndStageTable(ctx context.Context, catprops iceberg.Properties,
current *table.Table, ident table.Identifier, reqs []table.Requirement, updates
[]table.Update, cat table.CatalogIO) (*table.StagedTable, error) {
+// RejectUnlessConflict wraps non-conflict errors with table.ErrCommitRejected.
+// Use it only before the catalog has written anything.
+func RejectUnlessConflict(err error) error {
Review Comment:
Non-blocking: this adds a `commit rejected: ` prefix to SQL, Glue, Hive and
Hadoop error messages. A one-line note in `releases.md` would help anyone who
matches on error text.
--
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]
