Copilot commented on code in PR #10480:
URL: https://github.com/apache/gravitino/pull/10480#discussion_r3527257892
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -1153,9 +1237,29 @@ CatalogWrapper createCatalogWrapper(
* @return The resolved properties.
*/
private Map<String, String> getResolvedProperties(CatalogEntity entity) {
- CatalogWrapper catalogWrapper =
loadCatalogAndWrap(entity.nameIdentifier());
- return catalogWrapper.classLoader.withClassLoader(
- cl -> catalogWrapper.catalog.properties(), RuntimeException.class);
+ Map<String, String> conf = entity.getProperties();
+ String provider = entity.getProvider();
+
+ if (!classLoaderSharingEnabled) {
+ IsolatedClassLoader classLoader = createClassLoader(provider, conf);
+ try {
+ BaseCatalog<?> catalog = createBaseCatalog(classLoader, entity);
+ return classLoader.withClassLoader(cl -> catalog.properties(),
RuntimeException.class);
+ } finally {
+ ClassLoaderPool.cleanupClassLoader(classLoader);
+ }
Review Comment:
getResolvedProperties() creates a temporary BaseCatalog via
createBaseCatalog() (which may initialize an AuthorizationPlugin) but never
closes the catalog instance before cleaning up the ClassLoader. Since
listCatalogsInfo() calls getResolvedProperties() for each catalog, this can
leak resources (e.g., authorization plugins) and hold ClassLoader references
longer than intended.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -1153,9 +1237,29 @@ CatalogWrapper createCatalogWrapper(
* @return The resolved properties.
*/
private Map<String, String> getResolvedProperties(CatalogEntity entity) {
- CatalogWrapper catalogWrapper =
loadCatalogAndWrap(entity.nameIdentifier());
- return catalogWrapper.classLoader.withClassLoader(
- cl -> catalogWrapper.catalog.properties(), RuntimeException.class);
+ Map<String, String> conf = entity.getProperties();
+ String provider = entity.getProvider();
+
+ if (!classLoaderSharingEnabled) {
+ IsolatedClassLoader classLoader = createClassLoader(provider, conf);
+ try {
+ BaseCatalog<?> catalog = createBaseCatalog(classLoader, entity);
+ return classLoader.withClassLoader(cl -> catalog.properties(),
RuntimeException.class);
+ } finally {
+ ClassLoaderPool.cleanupClassLoader(classLoader);
+ }
+ }
+
+ ClassLoaderKey key = buildClassLoaderKey(provider, conf);
+ PooledClassLoaderEntry poolEntry =
+ classLoaderPool.acquire(key, () -> createClassLoader(provider, conf));
+ try {
+ IsolatedClassLoader classLoader = poolEntry.classLoader();
+ BaseCatalog<?> catalog = createBaseCatalog(classLoader, entity);
+ return classLoader.withClassLoader(cl -> catalog.properties(),
RuntimeException.class);
+ } finally {
+ classLoaderPool.release(poolEntry);
+ }
Review Comment:
In the sharing-enabled branch of getResolvedProperties(), a temporary
BaseCatalog is instantiated and used to compute properties(), but the catalog
is never closed before releasing the pooled ClassLoader entry. This can leak
AuthorizationPlugin / ops resources and also retain references to the pooled
ClassLoader, undermining the intended Metaspace leak fix.
--
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]
