From 25385c8d76b0f134b59257345bc57b2d17cc5b4d Mon Sep 17 00:00:00 2001
From: Mike Walch <mwalch@apache.org>
Date: Tue, 7 Mar 2017 15:57:04 -0500
Subject: [PATCH] ACCUMULO-4596 Remove env variable from
 general.dynamic.classpaths

* Removed default value for general.dynamic.classpaths which used
  ACCUMULO_HOME env variale
* Create system property 'accumulo.dynamic.classpaths' that sets
  same default value as before in accumulo-env.sh
* User can set dynamic classpaths in either location.  If both are
  set, the values are appended.
---
 assemble/conf/log4j.properties                |  1 +
 assemble/conf/templates/accumulo-env.sh       |  3 +-
 .../apache/accumulo/core/conf/Property.java   |  5 ++--
 .../vfs/AccumuloVFSClassLoader.java           | 29 ++++++++++++++-----
 .../vfs/AccumuloVFSClassLoaderTest.java       | 10 +++++++
 5 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/assemble/conf/log4j.properties b/assemble/conf/log4j.properties
index 5e602f262cf..637b30a1e70 100644
--- a/assemble/conf/log4j.properties
+++ b/assemble/conf/log4j.properties
@@ -24,6 +24,7 @@ log4j.appender.console.layout=org.apache.log4j.PatternLayout
 log4j.logger.org.apache.accumulo.shell.Shell.audit=WARN
 
 log4j.logger.org.apache.accumulo.core.file.rfile.bcfile.Compression=WARN
+log4j.logger.org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader=WARN
 log4j.logger.org.apache.accumulo.test.TestRandomDeletes=WARN
 log4j.logger.org.apache.commons.vfs2.impl.DefaultFileSystemManager=WARN
 log4j.logger.org.apache.hadoop.io.compress=WARN
diff --git a/assemble/conf/templates/accumulo-env.sh b/assemble/conf/templates/accumulo-env.sh
index a58d8164c9d..7622d394087 100644
--- a/assemble/conf/templates/accumulo-env.sh
+++ b/assemble/conf/templates/accumulo-env.sh
@@ -48,7 +48,8 @@ JAVA_OPTS=("${ACCUMULO_JAVA_OPTS[@]}"
   '-XX:OnOutOfMemoryError=kill -9 %p'
   '-XX:-OmitStackTraceInFastThrow'
   '-Djava.net.preferIPv4Stack=true'
-  "-Daccumulo.native.lib.path=${lib}/native")
+  "-Daccumulo.native.lib.path=${lib}/native"
+  "-Daccumulo.dynamic.classpaths=${lib}/ext/[^.].*.jar")
 
 ## Make sure Accumulo native libraries are built since they are enabled by default
 ${bin}/accumulo-util build-native &> /dev/null
diff --git a/core/src/main/java/org/apache/accumulo/core/conf/Property.java b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
index 6ff2bed42a2..430fbbabc88 100644
--- a/core/src/main/java/org/apache/accumulo/core/conf/Property.java
+++ b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
@@ -174,8 +174,9 @@ public enum Property {
       "A list of all of the places to look for a class. Order does matter, as it will look for the jar "
           + "starting in the first location to the last. Please note, hadoop conf and hadoop lib directories NEED to be here, "
           + "along with accumulo lib and zookeeper directory. Supports full regex on filename alone."), // needs special treatment in accumulo start jar
-  GENERAL_DYNAMIC_CLASSPATHS(AccumuloVFSClassLoader.DYNAMIC_CLASSPATH_PROPERTY_NAME, AccumuloVFSClassLoader.DEFAULT_DYNAMIC_CLASSPATH_VALUE,
-      PropertyType.STRING, "A list of all of the places where changes in jars or classes will force a reload of the classloader."),
+  GENERAL_DYNAMIC_CLASSPATHS(AccumuloVFSClassLoader.DYNAMIC_CLASSPATHS_SITE_PROPERTY, "", PropertyType.STRING,
+                             "A list of all of the places where changes in jars or classes will force a reload of the classloader. "
+                             + "List will appended to value set by the JVM system property - " + AccumuloVFSClassLoader.DYNAMIC_CLASSPATHS_JVM_PROPERTY),
   GENERAL_RPC_TIMEOUT("general.rpc.timeout", "120s", PropertyType.TIMEDURATION, "Time to wait on I/O for simple, short RPC calls"),
   @Experimental
   GENERAL_RPC_SERVER_TYPE("general.rpc.server.type", "", PropertyType.STRING,
diff --git a/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java b/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java
index f287364ca14..40ab5b456d8 100644
--- a/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java
+++ b/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java
@@ -82,14 +82,10 @@ public void run() {
   private static List<WeakReference<DefaultFileSystemManager>> vfsInstances = Collections
       .synchronizedList(new ArrayList<WeakReference<DefaultFileSystemManager>>());
 
-  public static final String DYNAMIC_CLASSPATH_PROPERTY_NAME = "general.dynamic.classpaths";
-
-  public static final String DEFAULT_DYNAMIC_CLASSPATH_VALUE = "$ACCUMULO_HOME/lib/ext/[^.].*.jar";
-
+  public static final String DYNAMIC_CLASSPATHS_JVM_PROPERTY = "accumulo.dynamic.classpaths";
+  public static final String DYNAMIC_CLASSPATHS_SITE_PROPERTY = "general.dynamic.classpaths";
   public static final String VFS_CLASSLOADER_SYSTEM_CLASSPATH_PROPERTY = "general.vfs.classpaths";
-
   public static final String VFS_CONTEXT_CLASSPATH_PROPERTY = "general.vfs.context.classpath.";
-
   public static final String VFS_CACHE_DIR = "general.vfs.cache.dir";
 
   private static ClassLoader parent = null;
@@ -171,8 +167,27 @@ static FileObject[] resolve(FileSystemManager vfs, String uris, ArrayList<FileOb
     return classpath.toArray(new FileObject[classpath.size()]);
   }
 
+  protected static String addToClasspath(String classpath, String addition) {
+    boolean cpExists = classpath != null && !classpath.isEmpty();
+    boolean adExists = addition != null && !addition.isEmpty();
+    if (cpExists && adExists) {
+      return classpath + "," + addition;
+    } else if (cpExists) {
+      return classpath;
+    } else if (adExists) {
+      return addition;
+    }
+    return "";
+  }
+
   private static ReloadingClassLoader createDynamicClassloader(final ClassLoader parent) throws FileSystemException, IOException {
-    String dynamicCPath = AccumuloClassLoader.getAccumuloString(DYNAMIC_CLASSPATH_PROPERTY_NAME, DEFAULT_DYNAMIC_CLASSPATH_VALUE);
+    String dynamicClasspathJvm = System.getProperty(DYNAMIC_CLASSPATHS_JVM_PROPERTY, "");
+    String dynamicClasspathSite = AccumuloClassLoader.getAccumuloString(DYNAMIC_CLASSPATHS_SITE_PROPERTY, "");
+    String dynamicCPath = addToClasspath("", dynamicClasspathJvm);
+    dynamicCPath = addToClasspath(dynamicCPath, dynamicClasspathSite);
+    log.info("JVM property {} = {}", DYNAMIC_CLASSPATHS_JVM_PROPERTY, dynamicClasspathJvm);
+    log.info("Config property {} = {}", DYNAMIC_CLASSPATHS_SITE_PROPERTY, dynamicClasspathSite);
+    log.info("Derived dynamic classpath = {}", dynamicCPath);
 
     String envJars = System.getenv("ACCUMULO_XTRAJARS");
     if (null != envJars && !envJars.equals(""))
diff --git a/start/src/test/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoaderTest.java b/start/src/test/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoaderTest.java
index c8207625923..20a6c539999 100644
--- a/start/src/test/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoaderTest.java
+++ b/start/src/test/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoaderTest.java
@@ -204,4 +204,14 @@ public void testCacheDirectoryConfigured() throws Exception {
 
     Whitebox.setInternalState(AccumuloVFSClassLoader.class, "loader", (AccumuloReloadingVFSClassLoader) null);
   }
+
+  @Test
+  public void testAddToClasspath() {
+    Assert.assertEquals("jar1", AccumuloVFSClassLoader.addToClasspath("", "jar1"));
+    Assert.assertEquals("jar1,jar2", AccumuloVFSClassLoader.addToClasspath("jar1", "jar2"));
+    Assert.assertEquals("", AccumuloVFSClassLoader.addToClasspath("", ""));
+    Assert.assertEquals("", AccumuloVFSClassLoader.addToClasspath("", null));
+    Assert.assertEquals("", AccumuloVFSClassLoader.addToClasspath(null, null));
+    Assert.assertEquals("jar1", AccumuloVFSClassLoader.addToClasspath("jar1", ""));
+  }
 }