shaoyu-li opened a new pull request, #13058: URL: https://github.com/apache/gravitino/pull/13058
### What changes were proposed in this pull request? This implements the SPI proposed in the design discussion #12971, replacing the hardcoded table location resolution in `GenericCatalogOperations` with a pluggable, `ServiceLoader`-discovered SPI. **The SPI** (`catalogs/catalog-lakehouse-generic`, package `org.apache.gravitino.catalog.lakehouse.generic`): - `TableLocationProvider extends Closeable` — `name()`, `default initialize(Map<String,String> catalogProperties)`, `String provisionTableLocation(TableLocationContext)`, `void unprovisionTableLocation(TableLocationContext)`, `default close()`. - `TableLocationContext` — immutable context object with a builder, carrying `tableIdentifier()`, `tableProperties()` and `schema()`. One type is used for both operations so a future input becomes an extra accessor rather than a signature change. - `TableLocationProviderFactory` — looks a provider up by name via `ServiceLoader` (case-insensitive) and initializes it. - `DefaultTableLocationProvider` — registered as `default`, reproducing today's behaviour verbatim. **Wiring:** one provider instance per catalog, selected by the new catalog property `table-location-provider`. `provisionTableLocation` replaces the private `calculateTableLocation`; `unprovisionTableLocation` is invoked from `dropTable` and `purgeTable`. **A design point worth calling out explicitly:** `dropSchema(cascade = true)` previously bypassed the catalog-level `dropTable` and called `tableOps(tableIdent).dropTable(tableIdent)` directly. Hanging the unprovision callback off `dropTable`/`purgeTable` alone would therefore never fire on a cascading drop — an entire schema's worth of storage would leak silently, with no error. This PR routes the cascade through the catalog-level `dropTable` so every contained table is unprovisioned one by one. Incidentally, this also makes the cascade path invalidate `tableFormatCache`, which it previously skipped; that fix falls out of the routing change rather than being a separate edit. **Ordering and failure semantics:** the table properties (which carry the location to reclaim) are read *before* the removal, and `unprovisionTableLocation` is called *after* it, so a provider never reclaims the storage of a table that is still there. A provider that throws is logged at WARN — naming the table, the provider and the unreclaimed location — and the drop still reports success: the table is already gone, so failing the request would report a drop that did happen as unsuccessful and invite a retry that cannot undo anything. For the same reason a failed unprovision does not abort a cascading schema drop. Leaked storage is recoverable; a premature reclaim is data loss. ### Why are the changes needed? `calculateTableLocation` walks a fixed chain — table `location` property, then schema property plus table name, then catalog property plus schema and table name, then failure — and the chain is private and not extensible. Deployments that allocate storage through an internal path-allocation service cannot express that today. The two available workarounds are both poor: make every client call the allocator and pass an explicit `location` (which does not stop a client from passing an arbitrary path that bypasses the allocation policy), or fork the catalog. Symmetrically, dropping a table strands its storage, and the existing `DropTableEvent` cannot be used to reclaim it — it carries only the user, the identifier and an existence flag, not the location, and it fires after the metadata is already gone. Fix: #12429 Design discussion: #12971. That thread is still open and carries a few questions the design deliberately left to review; this PR implements one set of answers rather than settling them. In particular `unprovisionTableLocation` is abstract rather than a default no-op, so that a provider allocating from an external system has to state what happens when the table goes away instead of inheriting an empty body by accident, and the SPI lives in `catalog-lakehouse-generic` rather than a shared module. Happy to change either if reviewers prefer the other answer. ### Does this PR introduce _any_ user-facing change? One new catalog property: `table-location-provider`, optional, defaulting to `default` and **immutable** once the catalog is created (switching it would leave one catalog's tables spread across two location schemes). No behavioural change for existing catalogs: unset means `DefaultTableLocationProvider`, which reproduces the current three-level fallback — including trailing-slash normalization and the existing `IllegalArgumentException` message when no `location` is set anywhere. `docs/lakehouse-generic-catalog.md` gains the property row and a "Pluggable table location provider" section. The one adjacent change is the cascade routing described above: `dropSchema(cascade = true)` now goes through the catalog-level `dropTable`, so it also invalidates the table format cache. ### How was this patch tested? 25 new unit tests; the module goes from 42 to 67 tests, 0 failures, 0 skipped. - `TestDefaultTableLocationProvider` (8) — each level of the fallback, trailing-slash handling, null schema properties, the no-location-anywhere failure, the provider name, and that unprovision is a no-op. - `TestTableLocationProviderFactory` (6) — default lookup, case-insensitive matching, discovery of a custom provider via `ServiceLoader`, a fresh instance per call, and the unknown/blank-name failures. - `TestGenericCatalogOperations` (11 new) — location validation (verbatim, blank rejected), unprovision on drop and on purge, no unprovision for a non-existent table, cascading schema drop unprovisioning every table, and that a throwing provider neither fails the drop nor aborts a cascade. Fixtures `FakeTableLocationProvider` and `FakeTableDelegator` are registered through `src/test/resources/META-INF/services/`. Verified on `769455366`: ``` ./gradlew :catalogs:catalog-lakehouse-generic:spotlessApply ./gradlew :catalogs:catalog-lakehouse-generic:compileJava :catalogs:catalog-lakehouse-generic:compileTestJava -PskipITs ./gradlew :catalogs:catalog-lakehouse-generic:test -PskipITs # 67 passed, 0 failed, 0 skipped ./gradlew :catalogs:catalog-lakehouse-generic:javadoc # 0 warnings on the new files ``` -- 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]
