yuqi1129 opened a new pull request, #13466: URL: https://github.com/apache/gravitino/pull/13466
> **Merge order:** on current `main` this change makes `TestRelationalEntityStoreBatchGetLateFill.testBatchGetRemovesValueWrittenAfterClear` fail instead of hang, because that test is the violation this guard detects. #13463 fixes the test and should merge first; this PR's `:core:test` is red until it does. That is the guard working — the same violation previously produced no output at all and the job was cancelled after 1h47m. ### What changes were proposed in this pull request? `SegmentedLock.withGlobalLock` now checks `globalGate.getReadHoldCount() > 0` and throws `IllegalStateException` when the calling thread is already inside a `withLock` action on the same instance, before it requests the write lock. The javadoc records the new `@throws`. ### Why are the changes needed? The rule already existed in the javadoc, added with the gate in #13400: > Must not be called from inside a `withLock` action on the same instance: the read lock cannot upgrade to the write lock, so such a call would deadlock. Nothing enforced it, and the javadoc alone did not prevent the violation. Breaking it parked the thread forever, and a deadlock reports nothing at all — no failing test name, no stack trace, no clue which call is at fault. #13462 is what that cost: `:core:test` hung and CI cancelled `build (17)` on `cd72eac711` after 1h47m. Measured on this branch with #13463 reverted: the same violation now fails in 58s naming `TestRelationalEntityStoreBatchGetLateFill.testBatchGetRemovesValueWrittenAfterClear`, instead of hanging indefinitely. This is defense-in-depth, not a fix for anything currently broken — #13463 removes the only present violation. The check is O(1) on a field the lock already maintains, on a path that runs once per whole-cache clear, so it costs nothing measurable. Fix: #13465 ### Does this PR introduce _any_ user-facing change? No. No production path calls `clear()` from inside a `withLock` action: the only callers of `cache.clear()` are `EntityCacheChangeLogListener.onEntityChange` (poller thread; `target.clear()` runs in the catch block, after `withLock`'s `finally` released the read lock) and `RelationalEntityStore.clearCache()`. Nothing legitimate is refused. ### How was this patch tested? - New `TestSegmentedLock.testGlobalLockInsideSegmentOperationFailsFast`, written first and watched fail: without the guard it reports `withGlobalLock deadlocked inside a withLock action` after a 10s join timeout. It runs the re-entrant call on its own daemon thread and asserts liveness before touching the lock again, so a regression fails in ten seconds rather than hanging the suite — the very failure mode this guard exists to prevent. It also asserts `isClearing()` stays false and that an ordinary `withGlobalLock` still runs afterwards. - `./gradlew :core:test --tests 'org.apache.gravitino.cache.*' -PskipITs` — 75 tests, 0 failures, 31s (including `TestSegmentedLock`'s 16). - With #13463 applied on top locally, `./gradlew :core:test --tests 'org.apache.gravitino.cache.*' --tests 'org.apache.gravitino.storage.relational.TestRelationalEntityStore*' --tests 'org.apache.gravitino.storage.relational.TestEntityCache*' --tests 'org.apache.gravitino.storage.relational.TestEntityChangeLog*' -PskipITs` — 142 tests, 0 failures, 1m13s. -- 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]
