yuqi1129 opened a new pull request, #13737: URL: https://github.com/apache/gravitino/pull/13737
### What changes were proposed in this pull request? Remove TreeLock from the 30 call sites in `PartitionOperationDispatcher`, `TagManager`, `PolicyManager` and `OwnerManager`. The invariant that replaces each lock: | Class (sites) | Replaced by | |---|---| | `PartitionOperationDispatcher` (6) | Nothing needed: partition operations never write the entity store. The `CatalogWrapper` lease (#12403) keeps the connector alive. | | `TagManager` (13) | Strict insert, tag version CAS, metalake row `FOR SHARE` on create. Assignments lock the tag rows and the live endpoint in one transaction (#13004). | | `PolicyManager` (7) | Strict insert, policy version CAS (#12206). Policy rows are locked in the assignment transaction. | | `OwnerManager` (4) | One live owner per object, with object and principal rows fenced in the assignment transaction (#13002). The plugin call already ran outside the lock. | `getTagWithoutLock` and `getPolicyWithoutLock` are renamed to `loadTagEntity` and `loadPolicyEntity`, since there is no locked variant any more. The `OwnerManager` Javadoc and the Tree Lock section of `docs/gravitino-server-config.md` are updated to match. Not changed here: the order in which the authorization plugin receives `onOwnerSet` for concurrent owner changes of the same object. The removed owner locks were READ locks on the owner principal, so they never ordered those calls; this is tracked by #13178. ### Why are the changes needed? TreeLock is per JVM and protects nothing across servers. For these families the database already enforces every invariant the lock guarded, so the lock only adds contention. Part of #13731 (epic #13730). ### Does this PR introduce _any_ user-facing change? On a single server, concurrent writes to the **same** tag or policy no longer queue behind each other. The loser gets `OptimisticLockException` (HTTP 409, error code 1012), which is what multi-node deployments already return (#12341). No update is lost. `docs/how-to-use-gravitino-client.md` already describes how to handle this response. ### How was this patch tested? New tests, with a small `RaceTestUtils.runTogether` helper that releases the actions together through a barrier: - `TestPartitionOperationDispatcher#testPartitionOperationsDoNotWaitForTableTreeLock`: partition operations complete while another thread holds the table WRITE lock. This test fails on `main`. - `TestTagManager#testConcurrentCreateTagHasOneWinner` and `TestPolicyManager#testConcurrentCreatePolicyHasOneWinner`: 8 concurrent creates leave one winner, and the others get `AlreadyExists`. - `TestTagManager#testConcurrentDeleteTagAndAssociateLeavesNoAssignment`: 10 rounds of tag delete racing with tag assignment leave no assignment on a deleted tag. - `TestOwnerManager#testConcurrentSetOwnerKeepsOneOwner`: 10 rounds of concurrent `setOwner` with a user and a group leave exactly one live owner. `./gradlew :core:test -PskipITs --tests TestTagManager --tests TestPolicyManager --tests TestOwnerManager --tests TestPartitionOperationDispatcher` passes (43 tests, H2; run three times with `--rerun`). The related REST tests (`TestTagOperations`, `TestMetadataObjectTagOperations`, `TestPolicyOperations`, `TestOwnerOperations`, `TestPartitionOperations`) and `TestFutureGrantManager`/`TestOwnerEvent` also pass. These manager tests run on H2 only; the database fences they rely on are covered by the `@TestTemplate` service tests (`TestTagMetaService`, `TestPolicyMetaService`, `TestOwnerMetaService`, `TestLiveEndpointService`), which run on H2, MySQL and PostgreSQL in CI. -- 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]
