richiebachala opened a new pull request, #2248:
URL: https://github.com/apache/iceberg-go/pull/2248
Part of #1691. Adds the table-commit slice to the catalog conformance suite,
and fixes the divergence it found.
## Bug
SQL, Hive, Glue and Hadoop validate commit requirements locally and returned
the bare requirement error (`Requirement failed: current schema id has changed
...`). REST maps the same case, a 409, to `table.ErrCommitFailed`. As a result:
- **A stale writer is never retried on those catalogs.** `Table.doCommit`
only retries on `ErrCommitFailed`, so setting `commit.retry.num-retries` had no
effect when another writer had already committed. #944 wrapped the low-level
write race on each catalog (SQL's 0-row update, Glue's version conflict, Hive
lock contention), but not this earlier, more common check.
- **A commit to a dropped table did not report `ErrNoSuchTable`.** The
transaction's `AssertTableUUID` failed against nil metadata and surfaced as an
untyped error.
## Fix
One shared helper, `internal.ValidateRequirements`, now used by
`UpdateAndStageTable` (SQL, Hive, Glue) and Hadoop's `CommitTable`:
- a failed requirement on a **missing** table → `catalog.ErrNoSuchTable`
- any other failed requirement → `table.ErrCommitFailed`
This matches REST and Java (`CommitFailedException` /
`NoSuchTableException`).
**Behaviour change:** callers matching these errors with `errors.Is` now
succeed where they previously failed. The retry loop stays opt-in, since
`commit.retry.num-retries` defaults to 0. The `ErrCommitFailed` doc comment,
which said only REST wrapped the sentinel, is updated.
The client-side `Validate` in `table/transaction.go`, which checks a
transaction's own staged requirements, is unchanged.
## Conformance tests
| Test | Checks |
|---|---|
| `CreateTableWithProperties` | create-time properties survive a reload |
| `UpdateTableProperties` | set and remove through a transaction |
| `UpdateTableSchema` | added column becomes current, gets the next field ID
|
| `CommitWithStaleRequirement` | `ErrCommitFailed`, and the rejected update
is not applied |
| `CommitToDroppedTable` | `ErrNoSuchTable`, and the table is not recreated |
Before the fix, the last two failed on SQL and Hadoop with the untyped
errors above. Commits are split, so reviewers can check out `cfeae89` to see
that.
A table-driven unit test for `ValidateRequirements` covers Glue, which has
no conformance run.
## Testing
- `go test ./catalog/... ./table/...`: pass
- Conformance, all five new tests passing on every backend:
- SQL, Hadoop: plain `go test`
- REST, Hive: `-tags integration` against `internal/recipe`
- Full Hive integration suite: pass
- golangci-lint (pre-commit): pass
I couldn't run the REST, SQL and Hadoop *S3* integration suites locally. On
macOS the host can't reach MinIO's container IP, so they time out with `dial
tcp 172.19.0.4:9000: i/o timeout`. They fail the same way on unmodified `main`,
so I'm relying on CI for them.
--
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]
