ctubbsii commented on code in PR #33:
URL:
https://github.com/apache/accumulo-classloaders/pull/33#discussion_r2590149999
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/LocalCachingContextClassLoaderFactory.java:
##########
@@ -196,6 +205,16 @@ public void init(ContextClassLoaderEnvironment env) {
} catch (IOException | ContextClassLoaderException e) {
throw new IllegalStateException("Error creating base cache directory at
" + baseCacheDir, e);
}
+ try {
+ MBeanServer mbs = ManagementFactory.getPlatformMBeanServer();
+ mbs.registerMBean(new ContextClassLoaders(),
ContextClassLoadersMXBean.getObjectName());
+ } catch (MalformedObjectNameException | MBeanRegistrationException
+ | NotCompliantMBeanException e) {
+ throw new IllegalStateException("Error registering MBean", e);
+ } catch (InstanceAlreadyExistsException e) {
+ // instance was re-init'd. This is likely to happen during tests
+ // can ignore as no issue here
+ }
Review Comment:
What's this stuff for?
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/LocalCachingContextCleaner.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.classloader.lcc;
+
+import java.io.IOException;
+import java.lang.ref.Cleaner;
+import java.lang.ref.SoftReference;
+import java.net.URLClassLoader;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalCachingContextCleaner {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(LocalCachingContextCleaner.class);
+ private static final List<SoftReference<URLClassLoader>> LOADERS = new
ArrayList<>();
+ private static final Cleaner CLEANER = Cleaner.create();
+
+ public static void registerClassLoader(final URLClassLoader cl) {
+ LOADERS.add(new SoftReference<>(cl));
+ CLEANER.register(cl, () -> {
Review Comment:
This won't work. SoftReferences prevent the monitor object from becoming
phantom-reachable, a prerequisite to run the cleaner. Instead, look at what I
did in the CleanerUtil class in the main Accumulo code. In order to clean up
the Closeable, it has to be contained within the monitor object that becomes
phantom-reachable. And then, you can close the objects that were contained
within it. The main catch is that you can't have anything registered with the
cleaner referencing the object being monitored. So, you can't close the object
being monitored this way, but you can close what's inside it. So, you can close
a URLClassLoader held inside a LocalCachingContext, if the LocalCachingContext
becomes phantom-reachable, but you can't watch for the URLClassLoader to be
phantom reachable and then close it.
##########
modules/local-caching-classloader/src/main/resources/META-INF/services/org.apache.accumulo.start.spi.KeywordExecutable:
##########
@@ -0,0 +1,20 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+org.apache.accumulo.classloader.lcc.definition.ContextDefinition
Review Comment:
Manually adding this file is fine, but you could also have used the
auto-service plugin with the annotation to generate this during the build, so
we have a little more flexibility and can be assured that the file will be
updated if/when we change the class name, add more, etc.
--
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]
