[GitHub] [accumulo] dlmarion commented on a diff in pull request #3136: WIP - remove VFS and only search file system for jars for context classloading

[GitBox <gi...@apache.org>]

dlmarion commented on code in PR #3136:
URL: https://github.com/apache/accumulo/pull/3136#discussion_r1052144180


##########
start/src/main/java/org/apache/accumulo/start/classloader/vfs/ContextManager.java:
##########
@@ -19,95 +19,96 @@
 package org.apache.accumulo.start.classloader.vfs;
 
 import java.io.IOException;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
 import java.util.function.Supplier;
+import java.util.stream.Collectors;
 
-import org.apache.commons.vfs2.FileSystemException;
-import org.apache.commons.vfs2.FileSystemManager;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-@Deprecated
-class ContextManager {
+//@Deprecated
+public class ContextManager {
 
   private static final Logger log = LoggerFactory.getLogger(ContextManager.class);
 
+  public static final String CONTEXT_CLASSPATH_PROPERTY = "general.vfs.context.classpath.";

Review Comment:
   ```suggestion
     public static final String CONTEXT_CLASSPATH_PROPERTY = "general.context.classpath.";
   ```



##########
start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java:
##########
@@ -39,7 +39,7 @@
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
-@Deprecated
+//@Deprecated

Review Comment:
   I think we want to remove this class. The idea here is that Accumulo will use the normal JVM classloader and classpath mechanisms. 



##########
shell/src/main/java/org/apache/accumulo/shell/commands/ClasspathCommand.java:
##########
@@ -30,9 +30,9 @@ public class ClasspathCommand extends Command {
   public int execute(final String fullCommand, final CommandLine cl, final Shell shellState) {
 
     final PrintWriter writer = shellState.getWriter();
-    org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.printClassPath(s -> {
-      writer.print(s);
-    }, true);
+    // org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.printClassPath(s -> {
+    // writer.print(s);
+    // }, true);

Review Comment:
   I'm going to think about how we can still do this.



##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1092,21 +1079,8 @@ public enum Property {
           + "constraint.",
       "2.0.0"),
 
-  // VFS ClassLoader properties

Review Comment:
   GENERAL_CLASSPATHS property can be removed also.



##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1116,20 +1090,7 @@ public enum Property {
           + " `general.vfs.context.classpath.<name>.delegation=post`, where `<name>` is"
           + " your context name. If delegation is not specified, it defaults to loading"
           + " from parent classloader first.",

Review Comment:
   Suggest replacing description with the following as the delegation model no longer applies:
   
   Properties in this category define a classpath. These properties start with the prefix followed by a context name. The value is a comma separated list of URIs. For example, general.context.classpath.cx1=file:///opt/accumulo/lib/cx1/*.jar,file:///usr/lib/accumulo/lib/cx1/*.jar



##########
start/src/main/java/org/apache/accumulo/start/classloader/vfs/ContextManager.java:
##########
@@ -19,95 +19,96 @@
 package org.apache.accumulo.start.classloader.vfs;
 
 import java.io.IOException;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
 import java.util.function.Supplier;
+import java.util.stream.Collectors;
 
-import org.apache.commons.vfs2.FileSystemException;
-import org.apache.commons.vfs2.FileSystemManager;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-@Deprecated
-class ContextManager {
+//@Deprecated
+public class ContextManager {
 
   private static final Logger log = LoggerFactory.getLogger(ContextManager.class);
 
+  public static final String CONTEXT_CLASSPATH_PROPERTY = "general.vfs.context.classpath.";
+
   // there is a lock per context so that one context can initialize w/o blocking another context
   private class Context {
-    AccumuloReloadingVFSClassLoader loader;
+    URLClassLoader loader;
     ContextConfig cconfig;
     boolean closed = false;
 
     Context(ContextConfig cconfig) {
       this.cconfig = cconfig;
     }
 
-    synchronized ClassLoader getClassLoader() throws FileSystemException {
+    synchronized ClassLoader getClassLoader() throws IOException {
       if (closed) {
         return null;
       }
 
       if (loader == null) {
-        log.debug(
-            "ClassLoader not created for context {}, creating new one. uris: {}, preDelegation: {}",
-            cconfig.name, cconfig.uris, cconfig.preDelegation);
-        loader =
-            new AccumuloReloadingVFSClassLoader(cconfig.uris, vfs, parent, cconfig.preDelegation);
+        log.debug("ClassLoader not created for context {}, creating new one. uris: {}",
+            cconfig.name, cconfig.uris);
+        loader = new URLClassLoader(Arrays.stream(cconfig.uris.split(",")).map(url -> {
+          try {
+            return new URL("file://" + url);
+          } catch (MalformedURLException e) {
+            throw new RuntimeException(e);
+          }
+        }).collect(Collectors.toList()).toArray(new URL[] {}), parent);
       }
 
-      return loader.getClassLoader();
+      return loader;
     }
 
     synchronized void close() {
       closed = true;
-      if (loader != null) {
-        loader.close();
-      }
       loader = null;
     }
   }
 
   private Map<String,Context> contexts = new HashMap<>();
 
   private volatile ContextsConfig config;
-  private FileSystemManager vfs;
-  private ReloadingClassLoader parent;
+  private ClassLoader parent;
 
-  ContextManager(FileSystemManager vfs, ReloadingClassLoader parent) {
-    this.vfs = vfs;
+  public ContextManager(ClassLoader parent) {
     this.parent = parent;

Review Comment:
   Not sure we need to the parent argument, the parent classloader will be the ClassLoader for this class.



##########
core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java:
##########
@@ -106,11 +106,9 @@ public enum PropertyType {
           + "Substitutions of the ACCUMULO_HOME environment variable can be done in the system "
           + "config file using '${env:ACCUMULO_HOME}' or similar."),
 
-  // VFS_CLASSLOADER_CACHE_DIR's default value is a special case, for documentation purposes
-  @SuppressWarnings("removal")
   ABSOLUTEPATH("absolute path",
       x -> x == null || x.trim().isEmpty() || new Path(x.trim()).isAbsolute()
-          || x.equals(Property.VFS_CLASSLOADER_CACHE_DIR.getDefaultValue()),
+          || x.equals(Property.CONTEXT_CLASSPATH_PROPERTY.getDefaultValue()),

Review Comment:
   I think you can just remove line 113 with no replacement



##########
core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java:
##########
@@ -269,13 +267,13 @@ public void testGetByPrefix() {
     assertSame(pmC, pmD);
     assertEquals(expected1, pmC);
 
-    tc.set(VFS_CONTEXT_CLASSPATH_PROPERTY.getKey() + "ctx123", "hdfs://ib/p1");
+    tc.set(CONTEXT_CLASSPATH_PROPERTY.getKey() + "ctx123", "hdfs://ib/p1");

Review Comment:
   Suggest changing these file paths to use file:///



##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java:
##########
@@ -155,9 +155,6 @@ MiniAccumuloConfigImpl initialize() {
       @SuppressWarnings("deprecation")
       Property generalClasspaths = Property.GENERAL_CLASSPATHS;

Review Comment:
   This property can be removed also



##########
start/src/main/java/org/apache/accumulo/start/Main.java:
##########
@@ -94,28 +94,15 @@ public static void main(final String[] args) throws Exception {
   public static synchronized ClassLoader getClassLoader() {
     if (classLoader == null) {
       try {
-        classLoader = (ClassLoader) getVFSClassLoader().getMethod("getClassLoader").invoke(null);
+        classLoader = org.apache.accumulo.start.classloader.AccumuloClassLoader.getClassLoader();

Review Comment:
   I think this method can be removed.



##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -76,7 +79,11 @@ public static ClassLoader getClassLoader(String context) {
     if (context != null && !context.isEmpty()) {
       return FACTORY.getClassLoader(context);
     } else {
-      return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
+      try {

Review Comment:
   We use the ContextClassLoaderFactory to return ClassLoaders for a table context. If a non-context classloader is requested, then we can return the application classloader. I think we can just return `ClassLoader.getSystemClassLoader()` or `ClassLoaderUtil.class.getClassLoader()` here.



-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org