[Bhavdeep-Sai]

What changes were proposed in this pull request?

This PR fixes the Gravitino client lifecycle issue described in #13675.

BaseCatalog could retain a catalog instance backed by a GravitinoClient that was later evicted from the Caffeine client cache. The cache removal listener closes the evicted client's HTTP transport, leaving the retained catalog with a closed client.

This change:

• Dynamically re-resolves the Gravitino catalog through GravitinoCatalogManager when catalog operations are performed.
• Invalidates catalog cache entries associated with an evicted Gravitino client before closing the client.
• Prevents BaseCatalog from continuing to use a catalog backed by a closed client.
• Adds regression tests covering both client cache expiration and maximum-size eviction.

Why are the changes needed?

When a cached GravitinoClient is evicted, GravitinoCatalogManager closes the client and its underlying HTTP connection pool.

However, BaseCatalog could still hold a catalog instance backed by that evicted client. Subsequent metadata operations could therefore attempt to use a closed HTTP transport and fail.

Re-resolving the catalog after cache eviction ensures that operations use an active Gravitino client, while invalidating the corresponding catalog cache entries prevents stale catalog instances from being reused.

Fix: #13675

Does this PR introduce any user-facing change?

No.

This is a bug fix to the internal Gravitino client and catalog cache lifecycle. No user-facing API or configuration property is introduced, removed, or changed.

How was this patch tested?

Added regression tests covering:

• Client cache expiration.
• Client cache maximum-size eviction.
• Catalog cache invalidation when a client is evicted.

The following tests/checks were run:

• ./gradlew :spark-connector:spark-3.5:test --tests "org.apache.gravitino.spark.connector.catalog.TestBaseCatalogClientEviction" --tests "org.apache.gravitino.spark.connector.catalog.TestGravitinoCatalogManager"
• ./gradlew :spark-connector:spark-3.5:test
• ./gradlew spotlessApply
• git diff --check
• ./gradlew test -PskipITs -PskipDockerTests=false

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[yuqi1129]

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

   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

   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

   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

   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.

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.