LuciferYang opened a new pull request, #13166: URL: https://github.com/apache/gravitino/pull/13166
### What changes were proposed in this pull request? The client cache now holds each `GravitinoClient` in a wrapper whose `close()` delegates only on a successful compare-and-set, so the underlying client is closed at most once however many paths reach it. The removal listener closes on every `RemovalCause` again, and `GravitinoCatalogManager#close` still drains the cache on the calling thread before invalidating it, so a client the drain sees is closed before `close()` returns. `TestGravitinoCatalogManager` recorded only whether a client had been closed at least once, which tolerated the second close. It now records how many times each client was closed, and `testCloseClosesEveryCachedClient` asserts exactly one close per client both immediately after `close()` and again after draining the common pool. Drop the synchronous drain and the first assertion fails; drop the compare-and-set and the second reports a client closed twice. ### Why are the changes needed? `close()` closed every cached client on the calling thread and then called `clients.invalidateAll()`, whose removal listener closed the client again. Caffeine dispatches removal listeners on `ForkJoinPool.commonPool()`, so that second close landed on a pool thread after `close()` had returned. `TestBaseCatalogSecrets` shares one mocked `GravitinoClient` across its cases and re-stubs it in `setUpCatalog`, which also closes the manager. When the second `close()` lands between `gravitinoClient.loadCatalog(any())` and `thenReturn`, Mockito resolves the pending stubbing to `close` and the case fails with `CannotStubVoidMethodWithReturnValue`. Both the test and the manager live in `spark-connector/spark-common`, so this can fail on any Spark line. Skipping `RemovalCause.EXPLICIT` in the listener would have been the smaller change, but `close()` drains the cache and then invalidates it, and both traversals are weakly consistent. A client inserted while the drain runs is removed by the invalidation as an explicit removal, so a listener that skipped explicit removals would never close it. An idempotent close keeps the guarantee without depending on which cause a removal reports. One narrower window is unchanged by either approach and stays open: a client published after the invalidation's own traversal has passed its bin is never removed, so nothing closes it. Fix: #13165 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? `TestGravitinoCatalogManager#testCloseClosesEveryCachedClient` pins one close per cached client on both sides of the drain. `testClientCacheEvictsAndClosesEvictedClient` still covers the asynchronous close on size eviction. -- 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]
