yuqi1129 commented on issue #12429:
URL: https://github.com/apache/gravitino/issues/12429#issuecomment-5355764702
Thanks for the detailed write-up. I read the code behind every claim in it,
and they are all correct: the four-step chain in
`GenericCatalogOperations.java:291-322`, the `x-lance-table-location` header in
`docs/lance-rest-service.md:317`, `ManagedTableOperations.java:192-197` doing
nothing to storage, `DropTableEvent.java:29-53` carrying only
user/identifier/`isExists`, and the `ServiceLoader` precedents. Nothing here is
overstated.
I do want to push back on two readings of that code, and then on the shape
of the solution.
## Two places where the quote is right but the reading is off
**1. The `purgeTable` javadoc is not describing a missing feature.**
The issue quotes it as evidence of an architectural gap:
> It only removes the table metadata from the entity store. Physical data
deletion should be handled by the specific catalog implementation if needed.
But that sentence assigns responsibility rather than declining it. Core is
saying the catalog implementation should do this. And one catalog already does:
`FilesetCatalogOperations.java:672-697` calls `fs.delete(location, true)` when
a managed fileset is dropped.
So the real situation is not "Gravitino has no concept of releasing
storage". It is "the generic lakehouse catalog has not implemented the
responsibility core already assigned to it". That difference matters a lot: for
a deployment that just wants the bytes removed, the fix is to do what the
fileset catalog does, and no SPI is involved.
**2. The reason the event mechanism does not work is different from the one
given.**
The issue says events are observational and cannot supply a location. The
conclusion is right, but the reason is not. `EventListenerPlugin` has
`transformPreEvent(SupportsChangingPreEvent)`, whose javadoc says "any changes
to resources in the event will affect the subsequent operations", and the
Iceberg REST server already uses it to rewrite create-table requests
(`IcebergCreateTablePreEvent`).
It does not work for core tables only because `CreateTablePreEvent` does not
implement that interface, and `TableEventDispatcher.createTable` throws away
the return value of `dispatchEvent` and passes the original properties on. Both
are small changes. Worth fixing the wording, because a reviewer who checks will
conclude the alternative was not really examined.
## What the proposal is actually asking for
The issue bundles three different capabilities into one SPI:
1. **Derive** a path from deployment inputs (owning team, domain, region,
tier).
2. **Enforce** that a client cannot pick an arbitrary path.
3. **Allocate** with side effects: reserve the bucket, check quota, register
ownership.
Only (3) needs a plugin. (1) and (2) do not, and Gravitino already has
mechanisms for both.
**For (1):** the fileset catalog has placeholder templating.
`FilesetCatalogOperations.java:1272-1300` builds the substitution map from
every `placeholder-*` property at fileset, schema and catalog level, plus the
built-in catalog/schema/fileset names. A catalog-level template like
`s3://bucket/{{team}}/{{region}}/{{schema}}/{{table}}` with the client passing
`placeholder-team=ads` covers exactly the inputs this issue lists. It is a pure
function, there is nothing to fail, and the template lives in catalog config.
The generic lakehouse catalog simply does not have it.
**For (2):** this is a policy question, and Gravitino has a policy
framework. Policies attach to a catalog, schema or table and inherit down the
hierarchy, which is the same shape as "catalog template, schema override, table
override".
## The part I would think hardest about
Gravitino already has an established pattern for "an external system needs
to act on rules held in metadata". `docs/policies.md` states it directly as a
common use:
> Feeding an external enforcement or scheduling system that reads policies
from Gravitino rather than keeping its own copy of what applies where
The shipped example is `system_iceberg_compaction`: Gravitino stores the
rules, and an external table maintenance service reads them and acts.
The direction of that dependency is external system reads Gravitino. This
proposal reverses it: Gravitino calls an external system, synchronously, inside
a metadata write. That is not just one more extension point; it changes where
Gravitino sits in the architecture, from the authority other systems read to a
client of a system it does not own.
Neither cited precedent supports that direction. `CredentialProvider` is on
a read path and its failures are per request. `LakehouseTableDelegator` is
in-process with no I/O. Neither one puts a remote, side-effecting call inside a
metadata write.
Concretely, what comes with (3):
- **Availability coupling.** If the allocator is down, table creation fails.
The provider is configured per catalog, so that is every table in the catalog.
- **Latency** added to a metadata write, with no timeout, circuit breaker or
fallback specified.
- **Divergence in both directions, and the issue only covers one of them.**
Besides the drop callback that may be lost, there is a create-side leak that is
not mentioned: in `GenericCatalogOperations.java:239-243`,
`calculateTableLocation` runs *before* the table format is validated and before
`tableOps.createTable`. A provider that reserved a bucket can then hit a failed
format check, a `TableAlreadyExistsException`, or a failure inside the format
implementation, leaving the allocator holding a reservation for a table that
does not exist.
- **A reconciler is required anyway.** Since both directions can diverge,
the deployment must reconcile. And a reconciler that lists tables and their
locations through the public API can already find abandoned storage on its own.
That makes the drop callback an optimisation rather than a contract, and I
would rather see it described that way.
- **Locations stop being reproducible.** Two identical create requests can
produce different paths, which affects testing, disaster recovery and migration
between environments.
- **Plugin lifetime.** A `ServiceLoader` plugin living in a catalog
classloader has real hazards here; #11739 is the cautionary tale.
## Alternatives, and what each one covers
| | Covers | Cost |
|---|---|---|
| Bring placeholder templating to the generic lakehouse catalog | derive |
small, mirrors the fileset catalog |
| Location policy / validation, possibly as a built-in policy type | enforce
| medium, no side effects, no availability coupling |
| Implement drop-time deletion in `GenericCatalogOperations` | release, for
self-managed storage | small, mirrors the fileset catalog, and is what the core
javadoc already asks for |
| Add location and properties to `DropTableEvent` | release, for external
allocators | small, and useful to everyone since the event is hard to use for
auditing today |
| Client allocates, server validates (today's flow plus a policy check) |
derive + enforce + allocate | no new architecture |
| The SPI as proposed | all three | everything in the list above |
The first four together cover most of the stated motivation and carry none
of those costs. The "push the problem to the client" option is dismissed in the
issue because nothing stops a client from passing an arbitrary path, but a
policy check is exactly what stops that.
## The question that decides the design
**Why must the allocation be triggered by Gravitino rather than validated by
Gravitino?**
If the allocator can hand out a pool of prefixes in advance, or can accept
lazy registration driven by a reconciler, then client-side allocation plus
server-side validation is enough, and Gravitino stays a metadata service.
The SPI is only necessary if all of the following hold at once: allocation
must happen at the moment of create, the server must be the one to trigger it,
and the result cannot be known in advance. That case is never argued in the
issue, and it is the one thing that would justify the rest.
## Two things to fix either way
**The proposal keeps open the hole it wants to close.** The motivation says
nothing stops a client from passing an arbitrary path, but the wiring says the
provider runs only after the explicit table-level `location` check, so an
explicit location still wins. As specified, a client sending
`x-lance-table-location` bypasses platform policy exactly as it does today. If
server-side enforcement is a goal, whatever makes the decision has to see the
client-supplied location and be able to reject or replace it.
**`location` is a mutable property.**
`GenericTablePropertiesMetadata.java:47` declares it with `immutable = false`,
so `alterTable(setProperty("location", ...))` can repoint a table with the
provider never being told: the old allocation leaks and the new path never went
through policy. Leaving `RenameTable` out of scope is fine, since a rename does
not move data, but changing `location` does.
## Smaller points
- **Module.** The gap is in `ManagedTableOperations` in core, and filesets
have the same problem, which the issue defers. Putting the SPI in
`catalog-lakehouse-generic` makes a parallel fileset version likely later.
- **Lifetime.** `TableLocationProvider extends Closeable`, but who closes
it, and when relative to catalog reload? The cited
`LakehouseTableDelegatorFactory` keeps a static singleton map, while
`initialize(catalogProperties)` implies one instance per catalog.
- **Misconfiguration.** If `table-location-provider` names an implementation
that is not on the classpath, catalog initialisation should fail loudly.
Falling back to the built-in chain would be a silent policy bypass.
- **External tables.** `PROPERTY_EXTERNAL` exists and `location` is
documented as required for them. Is the provider consulted?
## Summary
The problem is real, especially on the drop side. But I would describe that
side as the generic lakehouse catalog not implementing a responsibility core
already assigned, rather than as a missing extension point.
Of the three capabilities bundled here, two already have mechanisms in
Gravitino that the issue does not discuss. The third, side-effecting
synchronous allocation, is the one that genuinely needs a plugin, and it is
also the one that reverses the direction of dependency Gravitino has
established elsewhere.
My suggestion would be to land the small, independently useful pieces first
(placeholders in this catalog, drop-time deletion, richer `DropTableEvent`),
treat location policy as its own discussion, and keep the SPI open until the
case for synchronous, server-triggered allocation is made.
Happy to be wrong on any of this if the allocator really cannot work any
other way. That is the part I would most like to hear more about.
--
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]
