laserninja commented on PR #10675: URL: https://github.com/apache/gravitino/pull/10675#issuecomment-5076334393
Thanks @lasdf1234. I've opened a design doc for review: https://github.com/apache/gravitino/pull/12195 On your CAS question, this turned out to be the crux of the design, so let me answer it directly. Per-table CAS works on every backend, that's how ordinary single-table commits work. The thing that differs is **multi-table atomic commit**, and there the backends are genuinely not equivalent. Checked against Iceberg 1.11.0: | Backend | Multi-table atomic commit | Why | |---------|---------------------------|-----| | `JDBC` | **Yes, we can build it** | The pointer is a row in `iceberg_tables`. N conditional `UPDATE ... WHERE metadata_location = :old` statements issued on one `Connection` with `setAutoCommit(false)` commit or roll back together. | | `REST` | **Yes, by delegation** | The whole transaction can be forwarded upstream as a single `POST /v1/{prefix}/transactions/commit`. Our guarantee then equals the upstream catalog's, no stronger. | | `HIVE` | **No** | `HiveTableOperations.doCommit` issues a per-table `alter_table` through HMS Thrift, each its own server-side transaction, guarded by `MetastoreLock` or `NoLock`. HMS exposes no multi-table transactional alter. | | `MEMORY` | Yes (JVM lock) | Test-only. | | `CUSTOM` | Unknown | Arbitrary `Catalog` impl, no capability contract. | So: **JDBC is the only backend where Gravitino itself can construct atomicity. REST can inherit it by delegating. HIVE cannot, without an HMS-side change.** Worth being precise about HIVE, since it's tempting to think locking solves it: taking `MetastoreLock` on all N tables before committing does remove concurrent-writer races, but it does not make the `alter_table` calls atomic. A failure after the third of five still leaves three tables advanced. The most honest label is "serialized best-effort", which is not what `commitTransaction` promises. That asymmetry is what the design is built around. Rather than one uniform best-effort path, it proposes an explicit per-backend capability, and gates the `/v1/config` advertisement of `Endpoint.V1_COMMIT_TRANSACTION` on it, so a client never sees the endpoint advertised unless the guarantee behind it is real. Backends that cannot support it return `501` instead of silently degrading. This follows what the config endpoint already does for view support via `getEndpoints(supportsViewOperations)`. Two implementation details I'd particularly like your view on: 1. **JDBC.** `JdbcUtil.updateTable` can't be composed into one transaction, because it borrows its own autocommit connection per call. `JdbcClientPool.run(Action)` gives us one `Connection` for a whole lambda, which is the primitive we need. The catch is that Iceberg's commit SQL constants are `private`, so we'd have to restate the statement. The doc proposes we also raise this upstream and add an equivalence test in the meantime so a schema change upstream fails our build rather than silently corrupting commits. 2. **REST.** `RESTCatalog.commitTransaction(List<TableCommit>)` looks like the obvious API but is not a faithful forward. Its only factory is `TableCommit.create(identifier, base, updated)`, which derives the payload via `UpdateRequirements.forUpdateTable(...)` and therefore **discards the client's own `UpdateRequirement`s**, besides needing N extra loads to obtain `base`. The doc proposes a thin `RESTCatalog` subclass that forwards the inbound `CommitTransactionRequest` unmodified instead, so the upstream enforces the client's intended preconditions. @FANNG1 @bharos the doc also addresses the two points you raised: it does not claim atomicity where the backend cannot provide it, and it adds per-table authorization. On the latter, the `@AuthorizationExpression` interceptor resolves entities from path parameters, and this endpoint carries its tables in the body, so the doc does authorization programmatically via `MetadataAuthzHelper.checkAccess` against the same expression `updateTable` uses, with that expression extracted into a shared constant so the two paths can't drift. Happy to close this PR while the design is under review and open a fresh implementation once it's agreed, or leave it open as a reference if you'd find that more useful. Just let me know which you prefer. -- 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]
