ashwinsangem commented on issue #10474:
URL: https://github.com/apache/gravitino/issues/10474#issuecomment-4441330322
# HA-Safe Metadata Concurrency for Gravitino
## 1. Overview (the whole idea)
**Problem.** `TreeLock` only orders threads **inside one JVM**. In **HA**,
many JVMs share one metadata database; each has its **own** lock tree, so two
servers can still corrupt hierarchy state.
**Approach — dual layer (not two TreeLocks: one DB step, then one in-memory
lock).**
| Layer | Role
|
| ---------------------- |
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
|
| **Cross-node (new)** | Use the **same JDBC metadata database** to record
who may run **structural** work on which part of the tree (lease row + prefix
rules; optional PostgreSQL advisory lock where safe). |
| **In-node (existing)** | Keep `**TreeLock` / `LockManager` /
`TreeLockUtils`** so threads on **this** JVM stay ordered **the same way as
today**. |
We **do not** default to ZooKeeper / Redis / etcd, and we **do not** drop
`TreeLock` in favor of DB-only `SERIALIZABLE` everywhere (see §7 for other
options).
**Structural only.** The new DB claim applies to work that changes **shape
or membership** of the tree (rename/move under a parent, cascade drops, creates
that add children, …). Simple property updates and most reads use lighter rules.
**One structural write, in order**
1. **Claim in the shared DB** so other nodes see it. Claim key = canonical
`**scope_path`** (same *idea* as today’s `TreeLock` leaf path — §5).
2. **Take exactly one `TreeLock*`*, same `NameIdentifier` + `LockType` the
dispatcher already picks (not a second policy).
3. **Run work:** **connector** (Hive / Iceberg / JDBC / …) +
`**EntityStore`** (Gravitino’s own metadata rows) — see §3.
4. **Release `TreeLock`, then release the DB claim.**
**Why DB before `TreeLock`?** Avoid holding the in-memory tree lock while
waiting on JDBC and blocking unrelated local threads.
**Reads.** No distributed lock on every read. Use **one snapshot
transaction** for multi-step **relational** loads so parent/child rows cannot
“tear.” Engine + store stays **best-effort** (not one atomic transaction).
**Rollout.** Feature flag **off** = today’s behavior. **On** for HA = **all
nodes** together; half-on / half-off brings races back.
---
## 2. Background: today’s code and what breaks
**Code-anchored** = you can check this in the repo:
`TreeLockUtils.doWithTreeLock` → `GravitinoEnv.lockManager()`
(`TreeLockUtils.java`, `GravitinoEnv.java`). `TreeLock#lock` reads ancestors,
read/write on the leaf (`TreeLock.java`). Dispatchers and managers listed in
#10474 call this; e.g. rename shifts the leaf in
`TableOperationDispatcher#alterTable` when `TableChange.RenameTable` is present.
**HA gap.** Two processes each pass `TreeLock` locally but both touch
`**EntityStore`** → cross-node races.
**Failures we close:** (a) conflicting structural writes, (b) torn
multi-query relational reads, (c) **zombie commits** after lease steal — fixed
with **fencing** (§4).
**Design rules (short):** distributed scope must **match** today’s
`TreeLock` leaf choices; **DB claim before `TreeLock`**; prefer **one fence
anchor** (lock row or parent), not a fence column on every `*_meta` row;
document engine-vs-store order and failures; **heartbeats + reclaimer** for
leases; warn on mixed flag rollout.
---
## 3. Flow, diagram
Use a thin facade (e.g. `MetadataConcurrencyCoordinator`) at **structural**
boundaries.
```mermaid
flowchart TB
subgraph node [Gravitino process]
API[REST / gRPC handler]
MCC[MetadataConcurrencyCoordinator]
TL[TreeLockUtils / LockManager]
EC[EntityStore + mappers]
CC[Catalog connector]
end
DB[(Relational metadata DB)]
API --> MCC
MCC -->|"Step 1: DB scope claim"| DB
MCC -->|"Step 2: one TreeLock, same rules as today"| TL
MCC --> EC
MCC --> CC
```
**Steps (same as §1):** (0) Request hits **REST/gRPC** — entry only, not
special to locking. (1) DB claim. (2) One `TreeLock`. (3) Connector +
`EntityStore`. (4) Release `TreeLock`, then DB claim.
| | **Connector**
| `**EntityStore`**
|
| ----------- |
-------------------------------------------------------------------------------------------------------
| ------------------------------------------------------ |
| **What** | Catalog plugin: real Hive / Iceberg / JDBC / …
(`doWithCatalog` → `doWithTableOps` → `TableCatalog` …). | Gravitino JDBC
metadata (`store.*`, `TableEntity`, …). |
| **Example** | Rename table in metastore.
| Update stored
name/namespace/audit. |
Connector and store use **different connections/protocols** — not one big
transaction. Pick engine-first vs store-first **per operation**; document
partial failure (#10474 honesty: we fully serialize **relational** cross-node
races; **end-to-end** with every engine needs connector-level discipline).
---
## 4. Fencing
Put a monotonic `**fence`** on the **lease row** (simplest: no DDL on every
entity table). After lease expiry, a stealer bumps `fence`; late commits from
the old holder must **fail** if they no longer match. Prefer **one** DB
transaction for the structural `EntityStore` updates guarded by that fence.
---
## 5. DB coordination: key, DDL, behavior
`**scope_path` (PK)** — canonical path string aligned with the `TreeLock`
leaf for that op (e.g. `/mlake/cat/db` for schema scope, `/mlake/cat` for
catalog). **Prefix = ancestor** → conflict if any **active** row is equal,
ancestor, or descendant path. `**holder_id`** — which JVM owns the row
(renew/release), not the tree key.
**Reference DDL (PostgreSQL-style; tune per dialect)**
```sql
CREATE TABLE gravitino_metadata_scope_lock (
scope_path VARCHAR(2048) NOT NULL,
holder_id VARCHAR(256) NOT NULL,
fence BIGINT NOT NULL DEFAULT 0,
lease_expires_at BIGINT NOT NULL,
acquired_at BIGINT NOT NULL,
updated_at BIGINT NOT NULL,
CONSTRAINT pk_metadata_scope_lock PRIMARY KEY (scope_path)
);
CREATE INDEX idx_metadata_scope_lock_expires ON
gravitino_metadata_scope_lock (lease_expires_at);
CREATE INDEX idx_metadata_scope_lock_path_prefix ON
gravitino_metadata_scope_lock (scope_path varchar_pattern_ops);
```
**Acquire (one txn):** scan for conflicting **unexpired** rows → `INSERT` or
steal after expiry → bump `fence` on steal. **MySQL/H2:** same idea; watch
index length on long paths.
**PostgreSQL optional fast path:** `pg_try_advisory_xact_lock` in the
**same** txn as structural writes — auto-released at commit; use only when
**one coarse key** matches the real scope, else use the lease table (advisory
locks don’t encode hierarchy by themselves).
**Stale leases:** holder heartbeats + background **reclaimer** + metrics.
---
## 6. Rollout, migration, tests
| Config | Meaning
|
| --------------------------------------------- |
---------------------------------------------------------------------------- |
| `gravitino.metadata.distributed-lock.enabled` | Master switch; ship
default **off**. |
| `gravitino.metadata.distributed-lock.backend` | `LEASE_TABLE` |
`PG_ADVISORY` | `AUTO`. |
| Metrics | e.g. acquire latency,
contention, fence rejects, steals, snapshot-read txns. |
**Rollback:** flag off → **TreeLock-only** (no redeploy). **Ops:** don’t run
HA mixed on/off long-term.
**Phases:** inventory `TreeLockUtils` call sites → ship DDL + coordinator
(no-op when flag off) → wrap structural paths + two-node tests → snapshot reads
→ optional PG advisory.
**Tests:** overlapping claims; steal + fence; DB claim before `TreeLock`;
torn-read under RR; flag-off compatibility.
---
## 7. Other options (why we didn’t default there)
| Option | Drawback for default
|
| ------------------------------------------------- |
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
|
| ZooKeeper / etcd / Redis | Operators must run and
secure **another** HA system beside Gravitino and the metadata DB; its outage
or partition blocks or confuses **all** lock holders, not just one feature.
Every incident is now “is it Gravitino, the DB, **or** the coord?” — wider
blast radius and longer MTTR. You still need **fencing / lease** discipline in
the app; the coord does not remove careful storage writes by itself.
|
| DB-only `SERIALIZABLE` (or equivalent) everywhere | Most real flows touch
**both** the external catalog and JDBC; they **cannot** be one database
transaction, so you still need an explicit cross-node story for the non-DB
half. Serialization conflicts mean **retries and tail latency** on hot parents
(schemas, catalogs) where many ops overlap. “Subtree shape” invariants map
awkwardly to row locks — easy to miss an edge case versus an explicit **path**
lease. |
| Single active writer (leader) | Only the leader may
accept writes; other nodes are **standby for metadata mutations**, so you pay
HA complexity without **parallel write** throughput. Failover must guarantee
**no split-brain** and clean handoff of in-flight work — harder to reason about
than **peer nodes + shared lease rows**. Read scaling and regional routing get
tangled (“writes to leader, reads where?”) unless you add more policy anyway. |
**Why the proposed default instead:** reuse the **metadata DB** you already
require, keep **`TreeLock`** for local threads, and add **only structural**
cross-node claims — smaller new moving parts and a clear flag rollback.
**Closing.** Keep **`TreeLock`** for local ordering; add a **small,
store-backed scope lease + fencing** for structural HA; tighten **relational
reads** with snapshot transactions; be explicit about **connector vs
`EntityStore`**.
--
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]
