laskoviymishka commented on code in PR #2350:
URL: https://github.com/apache/iceberg-go/pull/2350#discussion_r4431201438
##########
catalog/hadoop/tables.go:
##########
@@ -208,23 +208,23 @@ func (c *Catalog) CommitTable(ctx context.Context, ident
table.Identifier, reqs
} else {
baseMeta, err = table.NewMetadata(iceberg.NewSchema(0), nil,
table.UnsortedSortOrder, "", nil)
if err != nil {
- return nil, "", fmt.Errorf("hadoop catalog: failed to
create base metadata: %w", err)
+ return nil, "", fmt.Errorf("%w: hadoop catalog: failed
to create base metadata: %w", table.ErrCommitRejected, err)
Review Comment:
Could this go through `internal.RejectUnlessConflict` like lines 198 and
217?
[`CommitTable`](https://github.com/apache/iceberg-go/blob/6d2f9f0c07c7fd93b1b65d3fba055b74e77be707/catalog/hadoop/tables.go#L197-L228)
now marks rejections two ways, the helper and a hand-built `%w` here and at
222 and 227. Line 198's message also has no `hadoop catalog:` part, unlike the
rest.
```suggestion
return nil, "",
internal.RejectUnlessConflict(fmt.Errorf("hadoop catalog: failed to create base
metadata: %w", err))
```
##########
catalog/internal/update_stage_test.go:
##########
@@ -92,6 +92,69 @@ func
TestUpdateAndStageTableRejectsEmptyMetadataLocationForExistingTable(t *test
require.Contains(t, err.Error(), "invalid metadata location")
}
+func TestUpdateAndStageTableClassifiesFailures(t *testing.T) {
Review Comment:
Could Glue and Hive each get a `CommitTable` test for this? This one covers
the shared helper, but no test under `catalog/glue` or `catalog/hive`
references `ErrCommitRejected`. A staging failure asserting rejected, and a
post-write failure asserting it isn't (for Glue, a non-CME
[`UpdateTable`](https://github.com/apache/iceberg-go/blob/6d2f9f0c07c7fd93b1b65d3fba055b74e77be707/catalog/glue/glue.go#L566-L596)
error), would pin both sides per catalog.
##########
catalog/sql/sql_test.go:
##########
@@ -1732,6 +1732,43 @@ func (s *SqliteCatalogTestSuite)
TestStaleSchemaUpdateIsRefusedOnRecommit() {
s.Equal(peerMeta.MetadataLocation(), current.MetadataLocation(),
"nothing may be committed")
}
+func (s *SqliteCatalogTestSuite)
TestRejectedUpdateLeavesTransactionResumable() {
Review Comment:
Could we pin the other side too: a non-conflict failure after
`WriteMetadata`, say the UPDATE erroring, comes back as unknown state and not
`ErrCommitRejected`? As I read it, only the call order in
[`CommitTable`](https://github.com/apache/iceberg-go/blob/6d2f9f0c07c7fd93b1b65d3fba055b74e77be707/catalog/sql/sql.go#L903-L915)
keeps those errors out of the rejected path, which turns cleanup on. An `n ==
0` race wouldn't catch it, since
[`RejectUnlessConflict`](https://github.com/apache/iceberg-go/blob/6d2f9f0c07c7fd93b1b65d3fba055b74e77be707/catalog/internal/utils.go#L284-L292)
passes `ErrCommitFailed` through.
--
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]
