urvishdesai commented on issue #10474:
URL: https://github.com/apache/gravitino/issues/10474#issuecomment-4537475752
## Proposal: Storage-Level Optimistic Concurrency Control via Existing
`current_version` Column
### Analysis of the Current Implementation
The locking system has two layers:
**Layer 1 — TreeLock (in-process, the reported problem):**
`TreeLockNode` wraps a `ReentrantReadWriteLock`. `LockManager` holds a
JVM-heap tree of these nodes. `TreeLockUtils.doWithTreeLock` is the single
entry point across all 8+ dispatchers. All primitives are JVM-local — a WRITE
lock on server A is invisible to server B.
**Layer 2 — Entity store (already has OCC, currently underused):**
Every entity table already carries `current_version INT NOT NULL DEFAULT 1`.
Every UPDATE SQL in the mapper layer already checks this column in the `WHERE`
clause. From `CatalogMetaBaseSQLProvider.updateCatalogMeta`:
```sql
UPDATE catalog_meta
SET catalog_name = #{new.catalogName}, current_version =
#{new.currentVersion}, ...
WHERE catalog_id = #{old.catalogId}
AND current_version = #{old.currentVersion} -- OCC check already exists
AND deleted_at = 0
```
`CatalogMetaService.updateCatalogMeta` already reads the affected-row count:
```java
if (updateResult.get() > 0) {
return newEntity;
} else {
throw new IOException("Failed to update the entity: " + identifier); //
typed wrong
}
```
The root problem is not missing OCC — the version check is already written.
What is missing is: (1) a typed conflict exception so callers can distinguish a
version mismatch from a disk failure, and (2) a retry loop. The same pattern
holds across every entity type's service class.
**HA failure mode (concrete example):**
```
Server A: alterTable(metalake, catalog, db, t1)
→ acquires WRITE lock in A's LockManager ✓ (invisible to B)
Server B: alterTable(metalake, catalog, db, t1) ← same table
→ acquires WRITE lock in B's LockManager ✓ ← no conflict detected!
Both proceed → RelationalEntityStore sees interleaved writes → last write
wins
```
---
### Proposed Solution
Activate the OCC already built into the entity store. This aligns with
**Option B** from the issue, with the key difference that TreeLock is kept —
the two layers take clearly separated responsibilities:
| Layer | Scope | Mechanism |
|---|---|---|
| TreeLock | Same JVM | `ReentrantReadWriteLock` — serializes local writes
cheaply, no DB cost |
| Entity store OCC | Cross-node | `WHERE current_version = N` — catches
remote concurrent writes via DB atomicity |
On a single node, the OCC check never fires in practice. In HA, it catches
what TreeLock cannot see.
#### Key Components to Change
**Step 1 — New typed exception (one class, no dependencies):**
```java
public class OptimisticLockException extends RuntimeException {
public OptimisticLockException(String message) { super(message); }
}
```
**Step 2 — Throw it in service classes instead of the generic
`IOException`:**
```java
// Same change in CatalogMetaService, SchemaMetaService, TableMetaService, …
if (updateResult.get() > 0) {
return newEntity;
} else {
throw new OptimisticLockException(
"Concurrent modification detected on: " + identifier);
}
```
**Step 3 — Retry in `TreeLockUtils.doWithTreeLock` (the only change to the
lock layer):**
```java
public static <R, E extends Exception> R doWithTreeLock(
NameIdentifier identifier, LockType lockType, Executable<R, E>
executable) throws E {
TreeLock lock =
GravitinoEnv.getInstance().lockManager().createTreeLock(identifier);
try {
lock.lock(lockType);
if (lockType == LockType.READ) {
return executable.execute();
}
int maxRetries = 3;
for (int attempt = 0; ; attempt++) {
try {
return executable.execute();
} catch (OptimisticLockException e) {
if (attempt >= maxRetries) throw e;
LOG.warn("OCC conflict on {}, attempt {}/{}", identifier,
attempt + 1, maxRetries);
Thread.sleep(10L << attempt); // 10ms → 20ms → 40ms
}
}
} finally {
lock.unlock();
}
}
```
Each retry re-invokes `executable`, which re-reads the entity with its
current `current_version`, reapplies the change, and issues a fresh `UPDATE …
WHERE current_version = fresh_version`.
---
### Analysis of Correctness
**Leaf-level writes (alter, rename): fully eliminated**
Two servers race to update the same entity:
- Server A reads `current_version = 5`, issues `UPDATE … WHERE
current_version = 5` → succeeds, version becomes 6.
- Server B reads `current_version = 5`, issues `UPDATE … WHERE
current_version = 5` → 0 rows → `OptimisticLockException` → retries at version
6 → succeeds.
**Version coordination:** No application-level coordination is needed. The
`UPDATE ... WHERE current_version = N` executes under the database's internal
row-level write lock. Two concurrent writers on the same row are serialized by
the DB itself — only one can transition from version N to N+1. The other
observes 0 rows updated and retries. The sequence per row is strictly 1 → 2 → 3
→ ..., guaranteed by DB atomicity with no external coordinator.
**Crash safety:** There is no held lock to release on crash. An uncommitted
version check leaves nothing to clean up — other nodes simply proceed with a
fresh read.
#### Known gap: parent-level write locks for create/drop
`createTable`, `dropTable`, and `purgeTable` acquire a WRITE lock at the
schema level (parent), not the table:
```java
// createTable locks the schema, not the table
TreeLockUtils.doWithTreeLock(schemaIdentifier, LockType.WRITE, ...)
```
This guards against `dropSchema` racing with `createTable` under the same
schema, which could produce an orphaned table row. Row-level OCC does not
replicate this parent-level blocking.
However, this is a **pre-existing HA gap** — TreeLock's parent write lock
only blocks threads within the same JVM and has never protected against
cross-node races. Our proposal does not make this worse.
The correct fix — independent of lock backend — is an atomic
parent-existence check inside the child insertion:
```sql
-- createTable: verify schema is alive within the same transaction
INSERT INTO table_meta (table_id, table_name, schema_id, ...)
SELECT #{tableId}, #{tableName}, sm.schema_id, ...
FROM schema_meta sm
WHERE sm.schema_id = #{schemaId} AND sm.deleted_at = 0
-- 0 rows inserted if schema was concurrently dropped → NoSuchSchemaException
```
This is a targeted follow-up applicable regardless of which locking strategy
is chosen.
#### Side note: version increment inconsistency
`POConverters.updateCatalogPOWithVersion` and `updateMetalakePOWithVersion`
set `nextVersion = lastVersion` — no increment. Catalogs and metalakes rely on
a full-field WHERE clause for OCC rather than version bumping. Tables always
increment. This inconsistency is a pre-existing issue worth fixing uniformly
(always increment `current_version` on update) but is not blocking for this
proposal.
---
### Analysis of Performance Impact
| Approach | Per-write DB cost (no conflict) | Per-write DB cost (on
conflict) |
|---|---|---|
| Today (single-node TreeLock) | 0 extra round-trips | N/A — local lock
serializes |
| Option A — external lock (ZooKeeper/etcd) | +1 network RTT on every write
| +1 RTT always |
| soulmachine — JDBC lock table | +2 DB round-trips on every write (lock row
acquire + release) | +2 always |
| **This proposal — OCC** | **0 extra round-trips** | **+1 read per retry,
max 3 retries** |
OCC has the lowest average DB load because it only pays extra cost on an
actual conflict. For metadata operations — concurrent writes to the same entity
from different nodes — conflicts are rare in practice.
---
### Comparison with Other Proposed Solutions
#### Option A — External Distributed Lock (ZooKeeper / etcd / Redis)
Replaces TreeLock with a distributed lock service. Every write acquires a
lock path over the network before touching the DB.
| Dimension | Option A |
|---|---|
| New dependencies | ZooKeeper, etcd, or Redis — a new operational component
to deploy, monitor, and keep highly available |
| Schema changes | None |
| Config overhead | High — cluster addresses, session timeouts, retry
policies, auth |
| DB load per write | Unchanged — lock is external |
| Lock granularity | Matches current TreeLock hierarchy |
| Crash recovery | ZK ephemeral nodes expire automatically; etcd leases
expire; Redis TTL |
| Code change size | Large — new client library, connection management,
session handling |
| HA correctness | Full, if the lock service itself is HA |
**Key concern:** The lock service becomes a new single point of failure
unless it is itself deployed in HA, which significantly increases operational
complexity. Lock service network latency is added to every write path even when
there is no actual contention.
#### soulmachine — LockBackend SPI with JDBC Pessimistic Lock ([PR
#11020](https://github.com/apache/gravitino/pull/11020))
Introduces a `LockBackend` interface with `InProcessLockBackend` (default,
wraps existing TreeLock) and `JdbcLockBackend` (opt-in HA, uses `SELECT … FOR
SHARE / FOR UPDATE` on a new `gravitino_lock` table).
| Dimension | soulmachine |
|---|---|
| New dependencies | None — reuses commons-dbcp2 already on classpath |
| Schema changes | New `gravitino_lock` table |
| Config overhead | Medium — 6 new `gravitino.lock.backend.jdbc.*` keys to
opt in to HA |
| DB load per write | +2 round-trips on every write (lock row acquire +
release), even with no contention |
| Lock granularity | Matches current TreeLock hierarchy via lock path string
|
| Crash recovery | Requires TTL column + background reaper for crashed lock
holders |
| Code change size | Medium — 2 new interfaces, 2 new backend classes,
design doc |
| HA correctness | Full for leaf-level writes; parent-level gap is the same
pre-existing issue |
**Key concern:** The pessimistic approach serializes at the lock row before
every write — even when there is no concurrent access. This trades
unconditional DB overhead for deterministic blocking, the opposite of what OCC
offers. It also defers the actual correctness fix (CAS on the entity store) to
a "Phase B", acknowledging that the JDBC lock alone is not the complete answer.
#### This Proposal — OCC via Existing `current_version`
| Dimension | This proposal |
|---|---|
| New dependencies | None |
| Schema changes | None — `current_version` already exists in every entity
table |
| Config overhead | None — works automatically |
| DB load per write | 0 extra round-trips on the happy path; +1 read per
retry on conflict only |
| Lock granularity | Row-level for leaf writes; parent-level gap is the same
pre-existing issue |
| Crash recovery | Not needed — no lock is held between read and write |
| Code change size | Small — 1 exception class, ~5 line change per service
class, ~15 line change to `TreeLockUtils` |
| HA correctness | Full for leaf-level writes; parent-level gap addressed in
follow-up |
---
### Migration Strategy
| Phase | Change | User impact |
|---|---|---|
| 1 | Add `OptimisticLockException`; update service classes to throw it on
0-row updates | None — behaviorally equivalent to current `IOException` for
existing callers |
| 2 | Add retry loop to `TreeLockUtils.doWithTreeLock` for WRITE operations
| Single-node: no observable change. HA: leaf-level write races eliminated |
| 3 | (Follow-up) Atomic parent-existence check in `createTable` and similar
cascade operations | Closes the pre-existing orphan gap for concurrent
drop/create |
| 4 | (Follow-up) Standardize version increment across all entity types in
`POConverters` | Consistent OCC token semantics for all entities, not just
tables |
No configuration changes. No schema changes. No new dependencies. TreeLock,
LockManager, and all 8+ dispatchers compile and behave identically for
single-node users.
---
**Summary:** Option A requires a new operational dependency that itself
needs HA. soulmachine's approach is a clean abstraction but pays extra DB cost
on every write and defers the core correctness fix to a later phase. This
proposal is the smallest change that closes the actual race — it activates the
`current_version` CAS machinery already written into every entity table's
mapper, adds a typed exception, and adds a bounded retry. The result is zero
extra infrastructure, zero extra config, zero extra DB load on the happy path,
and automatic HA correctness.
--
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]
