LuciferYang commented on code in PR #13166:
URL: https://github.com/apache/gravitino/pull/13166#discussion_r4068988905
##########
docs/review-6c2878fb1.md:
##########
@@ -0,0 +1,51 @@
+# Review：6c2878fb1 — 跳过 EXPLICIT 修掉了双关，但换来一个漏关窗口
+
+## 一、改动概述
+
+`GravitinoCatalogManager` 的 client cache 原来在 removal listener 里无条件
`closeClient`，而 `close()` 已经先同步遍历关过一遍，于是每个 client 被关两次，第二次由 Caffeine 派发到
`ForkJoinPool.commonPool()`、在 `close()` 返回之后才跑。
+
+改动分两部分：生产代码让 removal listener 跳过 `RemovalCause.EXPLICIT`（只有 `close()` 会产生这个
cause）；测试把 `ClientFactory` 的 per-client `AtomicBoolean` 换成 `AtomicInteger`
计数，`testCloseClosesEveryCachedClient` 在 `close()` 之后 drain common pool，再断言每个
client 只关一次。
Review Comment:
Removed. That note described the rejected `RemovalCause.EXPLICIT`-skipping
approach; the shipped code closes on every removal and relies on
`CachedClient`'s CAS for idempotence, so the note no longer matches the diff.
##########
spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/catalog/TestGravitinoCatalogManager.java:
##########
@@ -145,7 +147,18 @@ void testCloseClosesEveryCachedClient() {
manager.close();
- assertEquals(3, clientFactory.closedCount());
+ // Exact already here: no client the shutdown drain saw may outlive
close().
+ assertEquals(
+ List.of(1, 1, 1),
+ clientFactory.closeCounts(),
+ "Shutdown must close each cached client on the calling thread");
+ // Caffeine dispatches removal listeners on the common pool, so a second
close would land after
+ // close() returned. Draining the pool makes that visible instead of
leaving it to timing.
+ ForkJoinPool.commonPool().awaitQuiescence(30, TimeUnit.SECONDS);
Review Comment:
Fixed. This test now builds the manager with a same-thread executor
(`Runnable::run`) for the client cache, so the removal listener runs inline
inside `invalidateAll()` instead of on the common pool. `close()` drains and
closes every client, then the listener fires for the same entries on the
calling thread, so the test asserts each client was closed exactly once with no
wait. It fails deterministically (`[2, 2, 2]`) if the CAS guard is removed, and
no longer depends on common-pool quiescence.
--
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]
