LuciferYang opened a new pull request, #14741:
URL: https://github.com/apache/gravitino/pull/14741
### What changes were proposed in this pull request?
- `BaseCatalog` no longer keeps the Gravitino catalog it loaded in
`initialize`. It records the identity it was initialized for, and before each
Gravitino request it looks the catalog up through `GravitinoCatalogManager`
with the new protected method `gravitinoCatalog()`. The Hive, Iceberg, JDBC,
Paimon and Glue catalogs call it instead of reading the removed protected field
`gravitinoCatalogClient`.
- The lookup goes through the client cache first, so a client that a Spark
catalog keeps using is no longer evicted for being idle.
- The catalog cache now stores each catalog with the client it was loaded
through. When that client is no longer the current one, because the client
cache evicted and closed it, the catalog is loaded again through the current
client. This also covers a catalog that finished loading after its client was
evicted.
- Lookups fail after the manager is closed instead of building a new client.
### Why are the changes needed?
The client cache closes the clients it evicts, but `BaseCatalog` kept the
catalog loaded at `initialize` and sent every request through its client
without touching the cache. Once a client went unused in the cache for
`spark.sql.gravitino.clientCacheTtlSec` (one hour by default), the next catalog
lookup, for example from a new session, evicted and closed it, and every Spark
catalog initialized with it failed with `IllegalStateException: Connection pool
shut down` until the application restarted.
Fix: #13675
### Does this PR introduce _any_ user-facing change?
- Spark catalogs keep working after their Gravitino client is evicted.
- The catalog cache expiry (`spark.sql.gravitino.catalogCacheTtlSec`, 300
seconds by default) now also applies to initialized Spark catalogs, so the
first request after it expires sends one more `loadCatalog` request.
- A request that is already running when the size limit
(`spark.sql.gravitino.clientCacheMaxSize`) evicts its client can still fail;
the next request uses a new client.
- Out-of-tree subclasses of `BaseCatalog` that read `gravitinoCatalogClient`
need to call `gravitinoCatalog()` instead.
### How was this patch tested?
- New `TestGravitinoClientEviction` drives the caches with a fake Caffeine
ticker and checks that:
- a Spark catalog keeps working after its idle client is evicted and
closed;
- a client in use does not expire;
- a catalog loaded through a client that was evicted during the load is
loaded again through the new client;
- catalogs listed by `loadRelationalCatalogs` are not loaded again;
- a reload that fails with `NoSuchCatalogException` fails the request with
that exception.
- `TestBaseCatalogAuthorization`, `TestBaseCatalogFunctions` and
`TestBaseCatalogTableChanges` override `gravitinoCatalog()` instead of setting
the removed field.
- Reproduced the bug on main with the Spark 3.5 Hive integration test
environment, a 10 second client TTL and two sessions: after the second session
looked a catalog up, the first one failed with `Connection pool shut down`.
With this change the first session keeps working.
--
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]
