FANNG1 opened a new pull request, #13426: URL: https://github.com/apache/gravitino/pull/13426
### What changes were proposed in this pull request? Replaces the `lance.schema-refresh-mode` switch with two explicit load paths, and lets each caller pick the one whose guarantee it needs. - **`SupportsLightTableLoad`** (new, in `org.apache.gravitino.connector`): an optional connector mixin whose `loadTableLight` reads Gravitino's stored metadata only — never opens the dataset, never checks the version, never writes back. - **`TableDispatcher.loadTableLight`**: a `default` method falling back to `loadTable`, overridden to forward by all four dispatchers (event → normalize → hook → operation). `TableOperationDispatcher` runs light through the *same* dispatch as the full load — same tree lock, same connector snapshot, same hidden-property masking — differing only in which connector method it calls and in skipping the column sync-back. - **`LanceTableOperations.loadTable`** is now unconditionally the full load: it opens the dataset and compares versions on every call. `loadTableInternal(..., forAlter)`, `SchemaRefreshMode` and `schemaRefreshMode()` are gone. - **Caller routing** on the Lance REST service: `describeTable` selects by the `loadDetailedMetadata` flag it already receives; `tableExists` / `dropTable` / `deregisterTable` and the `EXIST_OK` existence probe use the light load, which removes the `super.loadTable` workaround; `alterTable` uses the full load, which removes the `forAlter` flag. **One deviation from the sketch in the issue.** The issue suggests "an internal Lance-specific interface". Placing it in `lance-common` does not work: `IsolatedClassLoader.isCatalogClass` does not list `org.apache.gravitino.lance.*`, so the name is treated as shared and delegated to the server's ClassLoader — which has no `gravitino-lance-common.jar` (it ships only into `catalogs/lakehouse-generic/libs/` and `lance-rest-server/libs/`). Each ClassLoader would then define its own copy and the `instanceof` would silently be false in every real deployment, while every test passed, because tests run all catalogs in one ClassLoader. The interface therefore lives in `core`, alongside `CatalogOperations` and `GenericTable`, which cross the same boundary today. Two tests pin this down. ### Why are the changes needed? A single deployment-wide setting had to serve two callers that want opposite things: the engine-facing Lance REST hot path, where most requests need only location and storage options, and the governance-facing path, where a caller asking for a schema must not be handed a stale one. The intent was already on the wire (`loadDetailedMetadata`) and then discarded. Fix: #13340 ### Does this PR introduce _any_ user-facing change? Yes, three. 1. **`lance.schema-refresh-mode` is removed** (property, enum, property metadata, docs). It was never in a release — `git tag --contains` on the commit that introduced it returns nothing — so there is no migration path to keep. 2. **A full load now fails instead of silently returning stale metadata.** If the dataset cannot be read (storage unreachable, expired credentials, no `location`), `loadTable` throws `ConnectionFailedException` → error code `1007` on Gravitino REST, HTTP `503` on the Lance REST endpoints. Previously it logged at debug and returned the stored columns, which is exactly what makes a "fresh schema" promise meaningless. The message points the caller at the light load. `TableExceptionHandler` and `LanceExceptionMapper` gained the corresponding branches. 3. **Requests that do not return a schema keep working while the storage does not.** `DescribeTable` without detailed metadata, `TableExists`, `DropTable` and `DeregisterTable` no longer touch the dataset. A declared table (`lance.declared=true`) is exempt from (2): its dataset is not expected to exist yet. Not user-facing but worth noting: a standalone Lance REST service reaches Gravitino over REST, which exposes only the full load, so its requests always take the full path. Documented as a limitation. ### How was this patch tested? `./gradlew :core:test :catalogs:catalog-lakehouse-generic:test :lance:lance-common:test :lance:lance-rest-server:test :server:test -PskipITs` — green. New coverage: - **ClassLoader (`TestIsolatedClassLoader`)** — the load-bearing one. Packages a second copy of `SupportsLightTableLoad` into an isolated ClassLoader's own jar, asserts the copy *is* visible to it, and asserts the class it resolves is still the server's. A second test pins the interface to `gravitino-core`. - **Light contract (`TestLanceTableOperations`)** — never opens the dataset, never writes to the store, returns stored columns when the dataset has moved on, and still answers while `openDataset` throws. - **Full contract** — checks the version even when columns are stored; fails with `ConnectionFailedException` on an unreadable dataset and on a missing `location`, with the light load still working in both; declared tables survive an unreadable dataset. - **Dispatcher (`TestTableOperationDispatcher`)** — light reaches the connector's light method and comes back through the same masking; the full load stays distinguishable; the `TableDispatcher` default falls back to `loadTable`. - **Routing (`TestGravitinoLanceTableOperations`)** — `describeTable` asks for a fresh schema only when it returns one; `tableExists` / `dropTable` / `deregisterTable` never do. ### Known gaps, deliberately out of scope - **Indexes are not refreshed.** The issue describes the full load as guaranteeing "schema and indexes"; this PR syncs columns and `lance.version` only — `replaceColumnsFromDataset` carries the stored indexes through untouched. Index sync belongs with the schema updater in #13339. - **`lance.version` can be stamped from derived columns.** `alterTable` (and `createTable`) record the post-change dataset version alongside columns derived from the `TableChange` array rather than re-read from the dataset. Any Gravitino↔Arrow conversion asymmetry there — the class of bug a021fe4a5 just fixed — would be frozen in, because the full load trusts a matching version. A `NOTE` comment marks the assumption. Fixing it is a one-line change (drop the version stamp in `alterTable` and let the next full load repair), but it is a separate behavioral change and was deferred. -- 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]
