LuciferYang commented on PR #10480: URL: https://github.com/apache/gravitino/pull/10480#issuecomment-4090635847
> We had tried it in this way, see #2644, but aborted it as a potential risk exists in classloader sharing, including > > * Static value in the classloader may be changed by one catalog and used by another > * Properties in classloader initialization may affect its behaviour; however, it can't be changed, although another catalog with the same type has different properties. > > These two factors mentioned above will affect the correctness of the Gravitino catalogs, so we hesitate to move on. Thank you for the context on #2644. The three concerns raised by @jerryshao are valid for that PR's approach — here's how this PR differs and addresses each one. --- ### Background: how this PR differs from #2644 PR #2644 proposed reusing ClassLoaders during **alter** operations on a **single catalog**, passing the old ClassLoader to the new `CatalogWrapper`. This created complex lifecycle issues — especially when catalog names change (cache invalidation closes the wrapper, which would close the shared ClassLoader) and when altered properties require a fresh ClassLoader. This PR takes a fundamentally different approach: a **reference-counted pool** that shares ClassLoaders across **different catalog instances** based on a composite key. The alter path is unaffected — alter still invalidates the old cache entry (releasing the pool reference) and creates a new wrapper (acquiring from the pool). The pool handles whether to reuse or create a new ClassLoader based on whether the key changed. --- ### Addressing the three specific concerns **1. "The classloader reuse for alteration makes the code a little broken"** This PR does not change the alter path's control flow. When a catalog is altered: - The old `CatalogWrapper` is invalidated from the Caffeine cache → removal listener calls `wrapper.close()` → `pool.release(poolEntry)` decrements refCount - A new `CatalogWrapper` is created via `createCatalogWrapper()` → `pool.acquire(key, factory)` either reuses an existing ClassLoader (if other catalogs share the same key) or creates a new one - The alter code in `CatalogManager` remains unchanged — it still invalidates and recreates as before The complex `setShouldCloseClassLoader` / conditional invalidation pattern from #2644 is not needed because the pool's reference counting handles the lifecycle automatically. **2. "If we modify some properties that require rebooting and refreshing the classloader, with this we cannot support it"** This concern applies when alter changes properties that affect ClassLoader construction. In this PR, the `ClassLoaderKey` is built from the catalog's current properties at creation time. If an alter changes a property that is part of the key (e.g., `Catalog.PROPERTY_PACKAGE`, authorization plugin path, or Kerberos identity), the new wrapper will have a **different key** and get a **fresh ClassLoader**. The old ClassLoader's refCount is decremented by the close of the old wrapper, and will be cleaned up when no other catalogs reference it. If the alter changes a property that is NOT part of the key (e.g., metastore URI, JDBC URL), the ClassLoader is reused — which is correct, because these properties do not affect ClassLoader construction (they are consumed by `CatalogOperations.initialize()`, which runs per-instance on the new wrapper). **3. "Sharing classloader between catalogs is dangerous, because some static variables that were created based on logic A will be used by another catalog that are based on logic B"** I audited all catalog implementations for static mutable state: - **Instance-level state (safe):** All catalog operations classes store runtime configuration in instance fields — `dataSource`, `clientPool`, `icebergCatalogWrapper`, `hadoopConf`, `fileSystemCache`, etc. These are per-catalog-instance and are not affected by ClassLoader sharing. - **`UserGroupInformation.loginUser` (addressed):** The only unkeyed static state that affects correctness. `ClassLoaderKey` includes Kerberos principal and keytab via isolation properties, so catalogs with different identities get separate ClassLoaders. - **`IcebergHiveCachedClientPool.clientPoolCache` (safe):** Static, but keyed by `(metastore URI, catalog name, user credentials)`. Different catalog configurations produce different cache entries — sharing the ClassLoader does not cause cross-contamination. - **`FileSystem.CACHE` (safe):** Static, but keyed by `(scheme, authority, UGI)`. Same isolation guarantee as above. The key insight is that ClassLoader construction in Gravitino depends only on JAR paths (provider + package + auth plugin), not on catalog runtime properties. Two catalogs with the same `ClassLoaderKey` load exactly the same classes — their "logic A" and "logic B" differ only at the instance level (different metastore URIs, different JDBC URLs, etc.), which is stored in instance fields and not affected by ClassLoader sharing. --- ### Future extensibility `ClassLoaderKey` stores isolation properties as a generic `Map<String, String>`, decoupled from any specific property names. This makes the pool infrastructure key-agnostic — only the logic that builds the key needs to know which properties matter. If new ClassLoader-scoped static state is discovered in the future, the isolation criteria can be extended by introducing a server configuration such as: ```properties gravitino.catalog.classloader.isolation.properties = authentication.type,authentication.kerberos.principal,authentication.kerberos.keytab-uri ``` `CatalogManager` would read this list at startup and extract matching values from catalog properties when building the key. Operators can then add isolation criteria for environment-specific static state without code changes or recompilation. The `ClassLoaderKey`, `ClassLoaderPool`, and `PooledClassLoaderEntry` require no modification — only the source of property keys changes. If the community prefers, I can implement this configurable approach in the current PR. -- 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]
