FANNG1 opened a new issue, #13339:
URL: https://github.com/apache/gravitino/issues/13339
### What would you like to be improved?
Discussion #12485 proposes treating the Gravitino entity store as a cache of
the Lance dataset schema. This issue covers one piece of that proposal: the
single schema updater.
Today the cached columns and the cached dataset version (`lance.version`)
are written from four different places in `LanceTableOperations`, each with its
own staleness rule and its own concurrency handling:
1. `createTable` — folds `lance.version` into the property map right after
`Dataset.write()`, before the entity is created
(`LanceTableOperations.java:408`).
2. `alterTable` — writes the dataset, then appends a synthetic
`TableChange.setProperty(lance.version, ...)` to the caller's change array and
hands it to `super.alterTable` (`:264-270`).
3. `repairTableMetadata` — on the `loadTable` path; replaces columns and
version under `updateTableWithCasRetry` (`:568`).
4. `recordCheckedEmptyVersion` — also on `loadTable`; writes the version
with a deliberately empty column list as a "confirmed empty at this version"
marker (`:646`).
What this costs us:
- **`loadTable` mutates.** Because repair happens inside load, other
operations deliberately route around it: `purgeTable` calls `super.loadTable`
to avoid a dataset open, `alterTable` calls `loadTableInternal(ident, true)`.
Every new caller has to know which load to use.
- **The alter path smuggles cache state through the user-facing change
list.** The version reaches the entity store as an ordinary property change,
indistinguishable from one the caller made.
- **The staleness rule is duplicated and not monotonic.**
`needsSchemaRefresh`, `isDatasetVersionChanged` and the inline check inside
`recordCheckedEmptyVersion` each decide "is the cache stale" slightly
differently, and `isDatasetVersionChanged` compares with `!=` (`:548`), so an
out-of-order write can move the cached version *backwards*. `alterTable` needs
its own `unhydratedSchema` guard (`:250-262`) precisely to stop a version write
from freezing an empty schema as if it were valid.
- **Concurrency handling is per-site.** Only the two load-path writers go
through `updateTableWithCasRetry`; `createTable` and `alterTable` do not.
### How should we improve?
Introduce one internal component that is the sole writer of cached schema
and version:
```
updater(table, version) bring this table's cached metadata to at least
`version`
idempotent, monotonic, no-op when already >=
version
```
- Full load calls it after reading the dataset; schema-altering operations
call it after writing the dataset. The four write points collapse into one.
- The staleness decision moves inside the updater — one rule, and `>=`
instead of `!=`, so the cached version can only move forward.
- CAS retry lives in the updater, so every writer gets the same concurrency
handling instead of two of the four having none.
- `loadTable` stops being a mutating operation from the caller's point of
view, which lets `purgeTable`'s `super.loadTable` workaround and `alterTable`'s
`loadTableInternal(ident, true)` flag go away.
- `alterTable` no longer appends a synthetic `TableChange`: schema changes
go to the dataset and then to the updater; non-schema changes (comment,
properties, rename) stay entirely in the entity store and never touch the
dataset.
- Because the updater is idempotent and monotonic, a failure there does not
have to fail the request — the cache simply lags until the next full load.
Scope: keep the updater internal to the Lance implementation, not a new
public catalog API. Per @yuqi1129's question on the discussion, the complete
call flow for load and alter, together with the concurrency, retry and failure
semantics, should be settled in review before implementation.
The other two pieces of #12485 — the light/full load split and separating
create / register / declare — are tracked separately; this issue does not
depend on either, though removing `SchemaRefreshMode` belongs with the
load-path split rather than here.
Related: #12485, #11295.
--
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]
