aisrefix-commits commented on issue #10474:
URL: https://github.com/apache/gravitino/issues/10474#issuecomment-4654162382
## Proposed direction: keep TreeLock, make the storage layer the cross-node
authority
After reading the existing proposals and walking the code, I'd like to float
a pragmatic, **incremental** direction and get feedback before implementing it
phase by phase.
**Overall idea (one sentence):** TreeLock stays as-is for intra-node
serialization; we wrap an "OCC conflict → bounded retry" loop around the
`executable.execute()` it already guards, so the DB's version check becomes the
final cross-node arbiter. Changes land in only two places — the storage layer
and the lock-utility entry point — while managers/dispatchers/SQL statements
stay essentially untouched.
This works because the storage layer already has the primitives, they're
just underused:
- Every entity table has `current_version`/`last_version`, and UPDATEs
already check them (`*MetaBaseSQLProvider`). What's missing is a **typed
conflict exception + a retry** — not the OCC guard itself.
- Transaction boundaries are centralized in `SessionUtils` (`doWithCommit`,
`doWithCommitAndFetchResult`, `doMultipleWithCommit`), so a guard can be
injected in one place.
- `EntityChangeLogPoller` + the `entity_change_log` mapper already exist on
`main` (mapper from merged #10914); it just isn't wired into `GravitinoEnv` yet.
### Races to close
| # | Race | Scenario | Root cause |
| --- | --- | --- | --- |
| R1 | alter/rename TOCTOU | Nodes A and B alter/rename the same entity; one
update silently overwrites the other | No cross-node lock; both pass their
local TreeLock |
| R2 | Orphaned child | A's `dropSchema` commits while B's `createTable`
under that schema also commits | create takes the schema's **in-process** write
lock; no cross-node parent guard |
| R3 | ABA lost update | metalake/catalog V1 → V2 → back to a V1-equivalent;
stale writer passes OCC | `nextVersion = lastVersion` (no increment) for
metalake/catalog |
| R4 | Stale cache | A commits a change; B keeps serving stale cached
metadata | `EntityChangeLogPoller` not wired into `GravitinoEnv` |
### Phased plan (each phase independently shippable & reversible)
**Phase 1 — Activate OCC (closes R1).** The key phase.
- Add a typed `EntityConcurrentModificationException` (~10 lines).
- In each `*MetaService` write, throw it on a 0-row update instead of the
generic `IOException` (`TableMetaService`, `SchemaMetaService`,
`CatalogMetaService`, `MetalakeMetaService`, `FilesetMetaService`,
`TopicMetaService`, `ModelMetaService`, …). Caveat: confirm 0-row means
"version mismatch" and not "entity gone"; if ambiguous, re-read before retrying
and throw `NoSuchEntityException` if it's gone.
- Add a bounded retry in `TreeLockUtils.doWithTreeLock`, **WRITE ops only**:
catch the exception → back off (10/20/40ms) → re-run the `executable`.
Precondition: the `executable` must be a "re-read → apply change → write"
closure so a retry sees the fresh `current_version`. Retry exhaustion → HTTP
409 via the server `ExceptionMapper`. Counts/backoff become config keys.
- Why it's sufficient: `UPDATE ... WHERE current_version = N` runs under the
DB row write lock; only one of two racers transitions `N → N+1`, the other gets
0 rows and retries at the fresh version. DB atomicity *is* the cross-node
serialization point — no external coordinator.
**Phase 2 — Version monotonicity (closes R3).**
- `POConverters.updateMetalakePOWithVersion` / `updateCatalogPOWithVersion`
increment `current_version` like `updateTablePOWithVersionAndSchemaId` already
does (today they set `nextVersion = lastVersion`). With strictly increasing
versions, an ABA cycle can't slip past OCC. No schema change.
- Risk point: confirm the metalake/catalog UPDATE `WHERE` actually carries
`current_version` (they rely on full-field matching today); if not, add the
version condition in `*MetaBaseSQLProvider` so OCC truly applies to these two
entity types.
**Phase 3 — Parent-existence guard for create/drop (closes R2).**
- create/drop take a **parent-level** write lock that row-level OCC can't
reproduce. Make the child insert atomically conditional on a live parent, in
the same transaction:
```sql
INSERT INTO table_meta (...)
SELECT ..., sm.schema_id, ...
FROM schema_meta sm
WHERE sm.schema_id = #{schemaId} AND sm.deleted_at = 0;
-- 0 rows inserted ⇒ schema concurrently dropped ⇒ NoSuchSchemaException
```
- One per parent→child relationship (schema→table/fileset/topic/model,
catalog→schema, metalake→catalog). Pure SQL, advanced entity by entity. (A FK
with `ON DELETE` semantics would be cleaner long-term but needs a cross-dialect
migration + soft-delete change — deferred.)
**Phase 4 — Cross-node cache coherence (closes R4).**
- Wire the existing `EntityChangeLogPoller` into `GravitinoEnv` behind a
default-off flag (e.g. `gravitino.cache.entityChangeLog.enabled`). Each node
polls `entity_change_log` on an independent cursor and invalidates the matching
cache entries. DB stays the source of truth; staleness bounded by the poll
interval. Default off ⇒ single-node behavior unchanged.
**Phase 5 (optional) — Pluggable `LockBackend` SPI (#11020).**
- Adopt the `LockBackend` abstraction: `InProcessLockBackend` (default,
wraps today's TreeLock) and an opt-in `JdbcLockBackend` (pessimistic `SELECT …
FOR UPDATE`/`FOR SHARE` on a path-keyed lock table) for deployments wanting
explicit coarse-grained cross-node locks. OCC stays the unconditional floor; a
JDBC backend would need a TTL + reaper for crashed holders. Stronger
active-standby topologies (leader election + epoch fencing) can layer above
this later.
### Why this over the alternatives
- **vs Option A (ZK/etcd per-write):** no new external HA dependency / SPOF,
and no network round-trip on every metadata op. A fencing token would still
have to propagate into each DB write to be correct — at which point the
external lock is largely redundant with the DB-level guard.
- **vs Option B (remove TreeLock):** keeps cheap, correct intra-node
serialization and avoids a big-bang refactor; pure row-level OCC can't express
coarse parent-level locks (handled here by the Phase 3 conditional insert).
- **vs Option C (partitioned ownership):** no
router/watchdog/grace-period/trigger machinery and a much smaller blast radius.
Performance-wise, OCC pays nothing on the happy path and only a re-read on
an actual same-entity conflict (rare for metadata), and it keeps unrelated
entities fully parallel — which matters for bursty concurrent DDL.
### Validation
Two-node, shared-DB integration tests exercising R1–R4 (concurrent rename of
the same table; `dropSchema` vs `createTable`; metalake/catalog ABA; cross-node
read-after-write under cache), plus OCC unit tests (forced 0-row update → retry
succeeds / exhausts to 409), and the full existing single-node suite with flags
off to confirm zero regression. Suggested metrics: OCC retry rate, change-log
poll lag (`maxId − lastConsumedId`), structural-op P99 latency.
---
I'm happy to start with **Phases 1+2** as the first reviewable PR — they're
the smallest change and close the main alter/rename + ABA races (~1 exception
class + 1 line per service + ~15 lines in `TreeLockUtils` + 2 spots in
`POConverters`). Does this overall direction sound reasonable before I proceed?
Feedback on any of the phases — especially the Phase 1 retry-closure assumption
and the Phase 2 `WHERE current_version` check — would be very welcome.
--
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]
