[GitHub] [accumulo] ctubbsii 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>]

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


##########
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 {

Review Comment:
   This class was deprecated with the expectation to remove it entirely. What do we still need it for?



##########
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
-
-  // this property shouldn't be used directly; it exists solely to document the default value
-  // defined by its use in AccumuloVFSClassLoader when generating the property documentation
-  @Deprecated(since = "2.1.0", forRemoval = true)
-  VFS_CLASSLOADER_SYSTEM_CLASSPATH_PROPERTY(
-      org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.VFS_CLASSLOADER_SYSTEM_CLASSPATH_PROPERTY,
-      "", PropertyType.STRING,
-      "Configuration for a system level vfs classloader. Accumulo jar can be"
-          + " configured here and loaded out of HDFS.",
-      "1.5.0"),
-  @Deprecated(since = "2.1.0", forRemoval = true)
-  VFS_CONTEXT_CLASSPATH_PROPERTY(
-      org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.VFS_CONTEXT_CLASSPATH_PROPERTY,
-      null, PropertyType.PREFIX,
+  // ClassLoader properties
+  CONTEXT_CLASSPATH_PROPERTY(ContextManager.CONTEXT_CLASSPATH_PROPERTY, null, PropertyType.PREFIX,

Review Comment:
   This is a new property. I don't think we need this. We are removing this feature in favor of the system classloader, which is already configurable by Java. We never need to specify a classpath in properties, just a context name on a per-table basis, which is interpreted based on the implementation details of the classloader factory, which should be pluggable SPI.



##########
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:
   Showing the contents of the `java.class.path` system property is probably enough here. For a configured context classloader factory, we could extend the SPI to have a method to show information about each context and call that here also. But, since this is run on the client side in the shell, it would have to be set up on the client side, and not just the server-side... unless we had a new RPC to specifically ask a random server to give us that info. And, in any case, we wouldn't be able to have much insight into the user's configured system classloader, if they set that... because that could do anything.



-- 
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