[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3136: Remove deprecated AccumuloVFSClassLoader

["ctubbsii (via GitHub)" <gi...@apache.org>]

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


##########
shell/src/main/java/org/apache/accumulo/shell/commands/ClasspathCommand.java:
##########
@@ -45,4 +45,22 @@ public String description() {
   public int numArgs() {
     return 0;
   }
+
+  public static void printClassPath(PrintWriter writer) {
+    try {
+      writer.print("Accumulo Shell Classpath: \n");

Review Comment:
   ```suggestion
         writer.println("Accumulo Shell Classpath:");
   ```



##########
start/src/main/java/org/apache/accumulo/start/Main.java:
##########
@@ -38,20 +37,17 @@ public class Main {
 
   private static final Logger log = LoggerFactory.getLogger(Main.class);
   private static ClassLoader classLoader;
-  private static Class<?> vfsClassLoader;
   private static Map<String,KeywordExecutable> servicesMap;
 
   public static void main(final String[] args) throws Exception {
     // Preload classes that cause a deadlock between the ServiceLoader and the DFSClient when
     // using the VFSClassLoader with jars in HDFS.
     ClassLoader loader = getClassLoader();
-    Class<?> confClass = null;
+    Class<?> confClass;
     try {
-      @SuppressWarnings("deprecation")
-      var deprecatedConfClass = org.apache.accumulo.start.classloader.AccumuloClassLoader
-          .getClassLoader().loadClass("org.apache.hadoop.conf.Configuration");
-      confClass = deprecatedConfClass;
-      Object conf = null;
+      confClass =
+          ClassLoader.getSystemClassLoader().loadClass("org.apache.hadoop.conf.Configuration");
+      Object conf;

Review Comment:
   Pretty sure this stuff can now be removed. It was added for a very specific workaround that I don't think applies anymore: https://issues.apache.org/jira/browse/ACCUMULO-4341



##########
shell/src/main/java/org/apache/accumulo/shell/commands/ClasspathCommand.java:
##########
@@ -45,4 +45,22 @@ public String description() {
   public int numArgs() {
     return 0;
   }
+
+  public static void printClassPath(PrintWriter writer) {
+    try {
+      writer.print("Accumulo Shell Classpath: \n");
+
+      final String javaClassPath = System.getProperty("java.class.path");
+      if (javaClassPath == null) {
+        throw new IllegalStateException("java.class.path is not set");
+      }
+      Arrays.stream(javaClassPath.split(File.pathSeparator)).forEach(classPathUri -> {
+        writer.print(classPathUri + "\n");
+      });
+
+      writer.print("\n");

Review Comment:
   ```suggestion
         writer.println();
   ```



##########
shell/src/main/java/org/apache/accumulo/shell/commands/ClasspathCommand.java:
##########
@@ -45,4 +45,22 @@ public String description() {
   public int numArgs() {
     return 0;
   }
+
+  public static void printClassPath(PrintWriter writer) {
+    try {
+      writer.print("Accumulo Shell Classpath: \n");
+
+      final String javaClassPath = System.getProperty("java.class.path");
+      if (javaClassPath == null) {
+        throw new IllegalStateException("java.class.path is not set");
+      }
+      Arrays.stream(javaClassPath.split(File.pathSeparator)).forEach(classPathUri -> {
+        writer.print(classPathUri + "\n");
+      });

Review Comment:
   ```suggestion
         Arrays.stream(javaClassPath.split(File.pathSeparator)).forEach(classPathUri -> writer.println(classPathUri));
   ```



##########
shell/src/main/java/org/apache/accumulo/shell/commands/ClasspathCommand.java:
##########
@@ -45,4 +45,22 @@ public String description() {
   public int numArgs() {
     return 0;
   }
+
+  public static void printClassPath(PrintWriter writer) {
+    try {
+      writer.print("Accumulo Shell Classpath: \n");
+
+      final String javaClassPath = System.getProperty("java.class.path");
+      if (javaClassPath == null) {
+        throw new IllegalStateException("java.class.path is not set");
+      }
+      Arrays.stream(javaClassPath.split(File.pathSeparator)).forEach(classPathUri -> {
+        writer.print(classPathUri + "\n");
+      });
+
+      writer.print("\n");
+    } catch (Exception t) {

Review Comment:
   I would just catch the checked exceptions here using multi-catch syntax, so you don't rewrap RTEs.



##########
shell/src/main/java/org/apache/accumulo/shell/commands/ClasspathCommand.java:
##########
@@ -45,4 +45,22 @@ public String description() {
   public int numArgs() {
     return 0;
   }
+
+  public static void printClassPath(PrintWriter writer) {
+    try {
+      writer.print("Accumulo Shell Classpath: \n");
+
+      final String javaClassPath = System.getProperty("java.class.path");
+      if (javaClassPath == null) {
+        throw new IllegalStateException("java.class.path is not set");
+      }
+      Arrays.stream(javaClassPath.split(File.pathSeparator)).forEach(classPathUri -> {
+        writer.print(classPathUri + "\n");
+      });
+
+      writer.print("\n");
+    } catch (Exception t) {
+      throw new RuntimeException(t);

Review Comment:
   Is there a more specific RTE type that we can use instead of this?



##########
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/impl/BlockCacheManagerFactory.java:
##########
@@ -31,7 +31,7 @@ public class BlockCacheManagerFactory {
 
   /**
    * Get the BlockCacheFactory specified by the property 'tserver.cache.factory.class' using the
-   * AccumuloVFSClassLoader
+   * configured ContextClassLoaderFactory

Review Comment:
   I think this will now use the system class loader. I don't think this is done inside a context, but I am not sure.



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