jerryshao commented on code in PR #12157: URL: https://github.com/apache/gravitino/pull/12157#discussion_r3664328586
########## design-docs/treelock-necessity-and-concurrency-design.md: ########## @@ -0,0 +1,526 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> + +# Design: Concurrency Control for Multi-Node Gravitino (TreeLock) + +> Tracking issue: [#10474](https://github.com/apache/gravitino/issues/10474) — *Address TreeLock limitations for Gravitino HA deployment* + +**Short words used in this doc:** + +- **HA** = High Availability = running more than one Gravitino server at the same time behind a load balancer. +- **OCC** = Optimistic Concurrency Control = the caller does not take an application/path lock before work; when it writes, it checks "is the row still the version I read?" If yes, write; if no, someone else changed it first, so handle the conflict. The database still takes its normal short statement/transaction locks. +- **External catalog** = the real system that holds the data, such as Hive, Iceberg, MySQL, or Kafka. +- **Gravitino store** = Gravitino's own database (`RelationalEntityStore`) that keeps a copy of the metadata. +- **Source of truth** = the system whose data we trust as correct when two copies disagree. + +--- + +## Background + +Gravitino uses an in-memory lock called `TreeLock` (`core/src/main/java/org/apache/gravitino/lock/`) to run metadata operations one at a time. For every operation, `TreeLockUtils.doWithTreeLock` locks the whole path from the root down: a read lock on every parent, and a read or write lock on the target (or on its parent for rename/drop). + +``` +loadTable(metalake.cat.db.t1) alterTable(... rename t1) dropTable(metalake.cat.db.t1) + / READ / READ / READ + /metalake READ /metalake READ /metalake READ + /metalake/cat READ /metalake/cat READ /metalake/cat READ + /metalake/cat/db READ /metalake/cat/db WRITE /metalake/cat/db WRITE + /metalake/cat/db/t1 READ (parent write-locked) (parent write-locked) +``` + +This is built on `LockManager`, which keeps an in-memory tree of `TreeLockNode`s (each one wraps a `ReentrantReadWriteLock`), plus reference counting, a background thread that removes unused nodes, and another background thread that checks for deadlocks inside the same JVM. It is about 800 lines of code in total (`LockManager` ~300, `TreeLockNode` ~250, `TreeLock` ~180, `TreeLockUtils` ~70). + +### The problem: TreeLock only works inside one JVM + +Each Gravitino server has its **own** `LockManager` with its own lock tree in its own memory. A write lock taken on server A means nothing to server B. Behind a load balancer, two servers can each pass their *local* TreeLock and change the same resource at the same time. So the moment Gravitino runs in HA, TreeLock stops protecting anything across servers. This is the reason #10474 was opened. + +This leaves us with a decision. There are two directions: + +1. **Remove TreeLock's correctness role** and let explicit rules in the shared database keep data correct. +2. **Keep the lock but make it work across nodes** (a distributed lock). + +The rest of this document analyses what TreeLock really does today, then compares these two directions, then picks one. + +--- + +## Goals + +1. **Understand what TreeLock protects today**: Describe what TreeLock actually guards, and which of those guarantees the shared database could provide instead. +2. **Evaluate the candidate directions on equal footing**: Assess each direction — database-native concurrency, and a cross-node distributed lock — against the same criteria (correctness, performance, maintainability, operational cost), informed by how comparable systems solve the two-store problem. Neither direction is assumed better going in. +3. **Correctness for one and for many servers**: Whatever is chosen must be correct with a single server and under HA. +4. **Decide with evidence**: End with a direction and the reasons behind it, plus a plan a developer can start on. Review Comment: I think one more goal is to keep the implementation simple and practically reasonable. We don't have to achieve strong consistency to make the implementation very hard to maintain. -- 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]
