Copilot commented on code in PR #11431:
URL: https://github.com/apache/gravitino/pull/11431#discussion_r3355964141
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/utils/ClassLoaderResourceCleanerUtils.java:
##########
@@ -87,23 +87,44 @@ private static void
closeStatsDataClearerInFileSystem(ClassLoader targetClassLoa
throws Exception {
Class<?> fileSystemClass =
Class.forName("org.apache.hadoop.fs.FileSystem", true,
targetClassLoader);
+
+ // If FileSystem was resolved from a parent/AppClassLoader rather than the
catalog's own
+ // classloader, its CACHE, Statistics cleaner, and MutableQuantiles
scheduler are shared
+ // across all catalogs in the JVM. Operating on shared static state here
would close every
+ // catalog's FileSystems and permanently terminate the global scheduler,
breaking any
+ // subsequent catalog that uses Hadoop metrics. Skip cleanup for shared
classes and let
+ // the JVM manage them.
+ if (!isOwnedByClassLoader(fileSystemClass, targetClassLoader)) {
+ LOG.debug(
+ "Hadoop FileSystem is owned by {}, not the target classloader {};
skipping shared-class cleanup",
+ fileSystemClass.getClassLoader(),
+ targetClassLoader);
+ return;
+ }
Review Comment:
The new shared-class ownership guards in the Hadoop/AWS/GCP/Azure cleanup
paths are not covered by tests. In unit tests, `closeClassLoaderResource`
short-circuits when `GRAVITINO_TEST` is set, so the production-only behavior
here can regress without detection. Please add focused tests (e.g., using a
custom parent/child ClassLoader setup and invoking the private cleanup methods
via reflection) to assert that static-state mutation is skipped when the
resolved class is parent-loaded, and performed when the class is
target-loader-owned.
--
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]
