LiJie20190102 commented on issue #12956:
URL: https://github.com/apache/gravitino/issues/12956#issuecomment-5649713155
@yuqi1129 Can you take a look at the solutions I have listed? If there's no
problem, I'll go ahead and do it.
## Proposal: Preserve ownership and recoverability during Lance table
overwrite
### Problem summary
OVERWRITE currently works as **drop-then-create** in
`LanceTableOperations.createTable`:
```java
// catalogs/catalog-lakehouse-generic/.../LanceTableOperations.java, lines
222–229
if (mode == CreationMode.OVERWRITE) {
if (register) {
dropTable(ident); // soft-delete metadata + delete Lance dataset
} else {
purgeTable(ident); // hard-delete metadata + delete Lance dataset
}
}
return createTableInternal(ident, columns, comment, properties, ...);
```
This causes two issues:
1. **Ownership hijack** — `dropTable`/`purgeTable` cascades a soft-delete of
the owner record and securable-object (privilege) relations
(`TableMetaService.deleteTableDependents`). The subsequent `createTable` call
in `TableHookDispatcher.createTable` sets the **current caller** as the new
owner. A caller with only `MODIFY_TABLE` can overwrite a table and become its
owner, then deregister/drop it — even though the original owner may never have
granted that.
2. **Data loss on failure** — the old table (metadata + Lance dataset) is
destroyed before the replacement is staged. If `createTableInternal` fails
(storage error, invalid schema, etc.), the original table and data are gone
with no recovery path.
### Proposed approach: stage-then-switch
Instead of drop+create, overwrite should **stage the replacement, then
switch metadata in place**, preserving the table's entity ID, owner, and
privilege associations.
#### Create path (non-register overwrite)
```
1. Try loadTable(ident)
├── not found → proceed with normal createTable (unchanged)
└── found → overwrite path:
2. Stage: write the new Lance dataset to a temporary location
(e.g. {tableLocation}.overwrite-{uuid})
3. Validate: open the staged dataset, read its schema and version
4. Switch metadata: update the existing TableEntity in-place via
store.update:
- replace columns from the new schema
- update lance.version
- update location to point at the staged dataset
- preserve table ID, audit creator, owner, and all privilege relations
Use the existing CAS-retry pattern (updateTableWithCasRetry) for OCC
safety.
5. Cleanup: delete the old Lance dataset at the original location
6. Rollback (if step 4 fails): delete the staged dataset, leave the
original table untouched
```
#### Register path (register overwrite)
Register overwrite replaces only the Gravitino metadata registration, not
the underlying data. The approach is simpler:
```
1. Try loadTable(ident)
├── not found → proceed with normal register (unchanged)
└── found → overwrite path:
2. Switch metadata: store.update the existing TableEntity with new properties
(new location, new lance.version). No Lance dataset staging needed —
the caller points at a different dataset path.
3. Rollback (if step 2 fails): nothing to clean up; original metadata is
intact
because store.update is atomic
```
### Why this works
- **Ownership preserved**: `store.update` modifies the existing
`TableEntity` (same ID, same namespace). It does **not** go through
`TableHookDispatcher.createTable`, so `setOwner` is never called. The owner
record and securable-object relations in the relational store are untouched.
- **Privileges preserved**: `deleteTableDependents` is never called, so
`softDeleteOwnerRelByMetadataObjectIdAndType` and
`softDeleteObjectRelsByMetadataObject` are not triggered.
- **Recoverability**: If the switch fails, the original TableEntity and
Lance dataset are both intact. The staged dataset is cleaned up. No partial
state.
- **OCC safety**: The switch uses the existing `updateTableWithCasRetry`
pattern (5 retries with randomized backoff), so concurrent overwrites on the
same table are handled by optimistic locking.
### Lance API notes
- `WriteParams.WriteMode` already has `CREATE`, `APPEND`, `OVERWRITE` —
currently only `CREATE` is used in `createTableInternal`. The staged dataset
can use `CREATE` mode at the temp location (we don't need Lance's native
overwrite; we stage and swap instead).
- `Dataset.drop(location, storageOptions)` is reused for cleanup of the old
dataset after a successful switch.
- `Dataset.commitOverwrite` exists as a native method but is not needed for
this approach.
### Code change scope
| File | Change |
|---|---|
| `LanceTableOperations.java` (catalog) | Rewrite the `OVERWRITE` branch in
`createTable` to stage+switch instead of drop+create. Add a `overwriteTable`
method that loads the old table, stages the new dataset, and updates metadata
via `store.update`. |
| `GravitinoLanceTableOperations.java` (REST common) | No change — `mode` is
still passed as `LANCE_CREATION_MODE` property. The catalog layer handles the
rest. |
| `LanceAuthorizationExpressions.java` | No change — OVERWRITE already
routes to `MODIFY_TABLE_AUTHORIZATION_EXPRESSION`. |
| `TableHookDispatcher.java` | No change — `createTable` is only reached for
genuinely new tables. |
### Test plan
| Scenario | What to verify |
|---|---|
| Retained ownership | Overwrite a table as a non-owner with `MODIFY_TABLE`;
verify `getOwner` returns the original owner, not the caller |
| Unchanged privileges | Overwrite and verify securable-object relations are
unchanged |
| Failed replacement preserves original | Inject failure during staging;
verify original table + data are intact |
| Concurrent overwrite | Two threads overwrite the same table; verify OCC
retry resolves and final state is consistent |
| Retry behavior | Simulate a transient `store.update` failure; verify CAS
retry succeeds |
| Create path overwrite | End-to-end: create table, overwrite with new
schema, verify data + schema changed, ownership preserved |
| Register path overwrite | End-to-end: register table, overwrite with new
location, verify metadata changed, ownership preserved |
### Open questions
1. **Column replacement semantics** — When switching metadata, should we
replace all columns atomically, or diff old/new schema and emit `TableChange`
operations? I lean toward atomic replacement via `store.update` (simpler, and
the old columns are going away anyway), but would like feedback.
2. **Location update** — Should the staged dataset be moved (rename) to the
original location, or should we update the table's `location` property to point
at the staged path? Rename is cleaner (no property change) but may fail on
cross-volume scenarios. Updating the property is more robust but leaves the old
path behind for manual cleanup.
3. **Register path** — Is the simplified approach (update properties only,
no staging) correct, or does register-overwrite also need to handle the case
where the caller wants to point at a dataset that doesn't exist yet?
--
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]
