[GitHub] accumulo pull request #231: ACCUMULO-4596 Remove env variable from general.d...

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #231: ACCUMULO-4596 Remove env variable from general.d...

ctubbsii
GitHub user mikewalch opened a pull request:

    https://github.com/apache/accumulo/pull/231

    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.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/mikewalch/accumulo dynamic-classpath

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/accumulo/pull/231.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #231
   
----
commit 25385c8d76b0f134b59257345bc57b2d17cc5b4d
Author: Mike Walch <[hidden email]>
Date:   2017-03-07T20:57:04Z

    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.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #231: ACCUMULO-4596 Remove env variable from general.d...

ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/231#discussion_r105064220
 
    --- Diff: 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")
    --- End diff --
   
    Do we even need this property? Shouldn't the default value simply be blank in the site file?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #231: ACCUMULO-4596 Remove env variable from general.d...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/231#discussion_r105064808
 
    --- Diff: start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java ---
    @@ -171,8 +167,27 @@ public void run() {
         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);
    --- End diff --
   
    This is a lot of added behavioral complexity. Part of the reason I thought we were trying to move away from reliance on environment values, is because of the behavioral complexity we had to incorporate in code in order to handle those environmental settings. Switching from an environment variable to a system property doesn't really achieve this goal, if we're including a bunch of new code to handle the special circumstances of that property.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo issue #231: ACCUMULO-4596 Remove env variable from general.dynamic....

ctubbsii
In reply to this post by ctubbsii
Github user mikewalch commented on the issue:

    https://github.com/apache/accumulo/pull/231
 
    @ctubbsii, I took a look at commons configuration 2 and agree that we should probably move to that first and then changedthe default for `general.dynamic.classpaths` to be based off a system property like `accumulo.lib.dir` set in accumulo-env.sh.  The default for `general.dynamic.classpaths` could then be `${sys:accumulo.lib.dir}/ext/[^.].*.jar`.  I will close this PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #231: ACCUMULO-4596 Remove env variable from general.d...

ctubbsii
In reply to this post by ctubbsii
Github user mikewalch closed the pull request at:

    https://github.com/apache/accumulo/pull/231


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---