yuqi1129 commented on PR #13677: URL: https://github.com/apache/gravitino/pull/13677#issuecomment-6008944570
Reviewed head `40520da259af78d8637fe21d6ed2a2b3eda07cf4`. The sequential size-eviction fix works, but I found four blocking issues: 1. **Eviction can miss a catalog that is still being loaded, allowing a closed-client catalog to be cached afterward.** [GravitinoCatalogManager.java, lines 252–257](https://github.com/apache/gravitino/blob/40520da259af78d8637fe21d6ed2a2b3eda07cf4/spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/catalog/GravitinoCatalogManager.java#L252-L257) A possible sequence is: A starts loading a catalog through client C; B evicts C, scans the catalog cache before A has published its entry, and closes C; A then finishes loading and publishes the catalog backed by C. The next lookup hits that cached catalog and fails with `Transport is closed`. I reproduced this with the actual manager and Caffeine cache, using latches to pause catalog loading, triggering expiration cleanup, and allowing the load to finish after the client was closed. The HTTP transport was mocked, so this is not a real-server integration test. Please bind catalog entries to the owning client instance/generation and protect resource use during loading and operations. Changing the invalidation/close order alone does not cover this sequence. 2. **The new unused variable prevents test compilation.** [TestGravitinoCatalogManager.java, line 187](https://github.com/apache/gravitino/blob/40520da259af78d8637fe21d6ed2a2b3eda07cf4/spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/catalog/TestGravitinoCatalogManager.java#L187) `bobCatalog` is never read. Running `./gradlew :spark-connector:spark-3.5:test -PskipITs -PskipDockerTests=true` on the unmodified head fails at `compileTestJava`: Error Prone reports `UnusedVariable`, and `-Werror` turns it into a compilation error. Keep the lookup call without assigning the result, or assert against the result. 3. **Both new BaseCatalog tests fail during initialization before reaching eviction.** [TestBaseCatalogClientEviction.java, lines 222–229](https://github.com/apache/gravitino/blob/40520da259af78d8637fe21d6ed2a2b3eda07cf4/spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/catalog/TestBaseCatalogClientEviction.java#L222-L229) The mock catalog does not stub `supportsSecrets()`, so it returns null. `BaseCatalog.initialize()` calls `propsWithSecrets()`, which immediately dereferences that result. After temporarily removing the compilation obstacle above, both new tests fail with this NPE. Please stub both secrets and credentials support, or explicitly simulate unsupported capability exceptions. 4. **The expiration test waits for removal without driving cache maintenance.** [TestBaseCatalogClientEviction.java, lines 94–97](https://github.com/apache/gravitino/blob/40520da259af78d8637fe21d6ed2a2b3eda07cf4/spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/catalog/TestBaseCatalogClientEviction.java#L94-L97) After repairing the mock fixture locally, this test still fails after its five-second wait. Polling `closedCount()` neither accesses the client cache nor invokes cleanup, and the cache has no scheduler. Caffeine expiration maintenance is driven by cache activity or `cleanUp()`; elapsed time alone does not guarantee the removal callback runs. Please use a controllable ticker plus explicit cleanup, then verify the same BaseCatalog can perform the next operation. See the [Caffeine 2.9.3 expiration documentation in source](https://github.com/ben-manes/caffeine/blob/v2.9.3/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java#L95-L101). **Validation:** With only the unused-variable obstacle removed and one review reproduction added, the Spark 3.5 non-integration suite ran 235 tests with three failures: the two fixture NPEs and the concurrent-loading reproduction. With the fixtures repaired, the two relevant test classes ran 13 tests with one failure (the expiration wait). The two size-eviction regression tests passed with the fix, both failed when the removal listener was reverted to closing without catalog invalidation, and passed again after restoring the listener. Spark 4.0, Docker, and integration tests were not run. All temporary source changes were restored after verification. -- 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]
