yuqi1129 opened a new pull request, #13463: URL: https://github.com/apache/gravitino/pull/13463
### What changes were proposed in this pull request? `TestRelationalEntityStoreBatchGetLateFill.testBatchGetRemovesValueWrittenAfterClear` advanced the cache invalidation epoch by calling `store.clearCache()` from inside `doPut`, i.e. inside the key's cache lock. Advance it with an unrelated-key invalidation (`store.delete`) instead, which does not take this key's lock and needs no global write lock, and rename the test to `testBatchGetRemovesValueWrittenAfterInvalidationDuringPut`. ### Why are the changes needed? Since #13400, `SegmentedLock.withLock` holds the `globalGate` read lock across its critical section and `withGlobalLock` takes the write lock. The read lock cannot upgrade, so the test's re-entrant `clearCache()` parks on itself forever and `:core:test` never finishes — `build (17)` on `cd72eac711` was cancelled after 1h47m. #13400's javadoc already forbids exactly this call pattern: > 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. Neither PR is wrong alone: #13374 added the re-entrant hook while `withGlobalLock` took no gate lock, and #13400 added the gate without knowing about the hook. Both were green in isolation. The re-check the test covers (`RelationalEntityStore.batchGet:289-293`) is still needed, but only for the case it can actually face: a hierarchical or unrelated invalidation that advances the epoch without taking the key's lock. #13374 itself calls that residual window out. A whole-cache clear can no longer interleave there, so simulating one no longer models anything reachable. Test-only; no production path calls `clear()` from inside a `withLock` action. The only production 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()`. Fix: #13462 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - The test now passes instead of hanging: the 8 `TestRelationalEntityStoreBatchGetLateFill` cases finish in 25s. - Mutation check that it is not neutered: temporarily deleting the inner re-check in `batchGet` makes it fail (`expected: <false> but was: <true>`); restoring the re-check makes it pass again. - `./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` — 141 tests, 0 failures, 57s (including `TestSegmentedLock`'s 15). -- 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]
