[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

joshelser
GitHub user mikewalch opened a pull request:

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

    Accumulo-4599 Simplify how accumulo script adds jars to classpath

    * Removed ACCUMULO_XTRAJARS variable
    * Include '-add' option in accumulo script usage

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

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

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

    https://github.com/apache/accumulo/pull/230.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 #230
   
----
commit 00f64a8e48818401543b0a596af3121df4cda60a
Author: Mike Walch <[hidden email]>
Date:   2017-03-07T15:29:29Z

    Accumulo-4599 Simplify how accumulo script adds jars to classpath
   
    * Removed ACCUMULO_XTRAJARS variable
    * Include '-add' option in accumulo script usage

----


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

joshelser
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/230#discussion_r104707884
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    -    export ACCUMULO_XTRAJARS="$2"
    +    CLASSPATH="${2}:${CLASSPATH}"
         shift 2
    -  else
    -    export ACCUMULO_XTRAJARS=""
       fi
       if [[ "$cmd" = "jar" && -f "$2" ]]; then
    -    if [[ $2 =~ ^/ ]]; then
    -      jardir="$(dirname "$2")"
    -      jarfile="$2"
    -    else
    -      jardir="$(pwd)"
    -      jarfile="${jardir}/${2}"
    -    fi
    -    if jar tf "$jarfile" | grep -q META-INF/MANIFEST.MF ; then
    -      cp="$(unzip -p "$jarfile" META-INF/MANIFEST.MF | grep ^Class-Path: | sed 's/^Class-Path: *//')"
    --- End diff --
   
    what was this for?


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

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

    https://github.com/apache/accumulo/pull/230#discussion_r104759592
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    -    export ACCUMULO_XTRAJARS="$2"
    +    CLASSPATH="${2}:${CLASSPATH}"
         shift 2
    -  else
    -    export ACCUMULO_XTRAJARS=""
       fi
       if [[ "$cmd" = "jar" && -f "$2" ]]; then
    -    if [[ $2 =~ ^/ ]]; then
    -      jardir="$(dirname "$2")"
    -      jarfile="$2"
    -    else
    -      jardir="$(pwd)"
    -      jarfile="${jardir}/${2}"
    -    fi
    -    if jar tf "$jarfile" | grep -q META-INF/MANIFEST.MF ; then
    -      cp="$(unzip -p "$jarfile" META-INF/MANIFEST.MF | grep ^Class-Path: | sed 's/^Class-Path: *//')"
    --- End diff --
   
    Not sure why it was put there.  It looks like it's adding the classpath in the manifest to the classpath which is strange.  A better way would be to create an uber jar or use `accumulo -add <extra-classpath> <main-class>` to run.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

joshelser
In reply to this post by joshelser
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/230#discussion_r104761530
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    -    export ACCUMULO_XTRAJARS="$2"
    +    CLASSPATH="${2}:${CLASSPATH}"
         shift 2
    -  else
    -    export ACCUMULO_XTRAJARS=""
       fi
       if [[ "$cmd" = "jar" && -f "$2" ]]; then
    -    if [[ $2 =~ ^/ ]]; then
    -      jardir="$(dirname "$2")"
    -      jarfile="$2"
    -    else
    -      jardir="$(pwd)"
    -      jarfile="${jardir}/${2}"
    -    fi
    -    if jar tf "$jarfile" | grep -q META-INF/MANIFEST.MF ; then
    -      cp="$(unzip -p "$jarfile" META-INF/MANIFEST.MF | grep ^Class-Path: | sed 's/^Class-Path: *//')"
    --- End diff --
   
    I agree, this would be better handled externally.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

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

    https://github.com/apache/accumulo/pull/230#discussion_r104786398
 
    --- Diff: start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java ---
    @@ -171,22 +171,10 @@ public void run() {
         return classpath.toArray(new FileObject[classpath.size()]);
       }
     
    -  private static ReloadingClassLoader createDynamicClassloader(final ClassLoader parent) throws FileSystemException, IOException {
    +  private static ReloadingClassLoader createDynamicClassloader(final ClassLoader parent) throws IOException {
         String dynamicCPath = AccumuloClassLoader.getAccumuloString(DYNAMIC_CLASSPATH_PROPERTY_NAME, DEFAULT_DYNAMIC_CLASSPATH_VALUE);
     
    -    String envJars = System.getenv("ACCUMULO_XTRAJARS");
    -    if (null != envJars && !envJars.equals(""))
    -      if (dynamicCPath != null && !dynamicCPath.equals(""))
    -        dynamicCPath = dynamicCPath + "," + envJars;
    -      else
    -        dynamicCPath = envJars;
    -
    -    ReloadingClassLoader wrapper = new ReloadingClassLoader() {
    -      @Override
    -      public ClassLoader getClassLoader() {
    -        return parent;
    -      }
    -    };
    +    ReloadingClassLoader wrapper = () -> parent;
    --- End diff --
   
    +1


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

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

    https://github.com/apache/accumulo/pull/230#discussion_r104785958
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    -    export ACCUMULO_XTRAJARS="$2"
    +    CLASSPATH="${2}:${CLASSPATH}"
         shift 2
    -  else
    -    export ACCUMULO_XTRAJARS=""
       fi
       if [[ "$cmd" = "jar" && -f "$2" ]]; then
    --- End diff --
   
    I think this `jar` command can be removed also. It's not useful if it's just yet-another-way to add items to CLASSPATH. Again, we have the env config 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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

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

    https://github.com/apache/accumulo/pull/230#discussion_r104786227
 
    --- Diff: start/src/main/java/org/apache/accumulo/start/Main.java ---
    @@ -212,6 +212,7 @@ public static void printUsage() {
           printCommand(executableMap.remove(cmd));
         }
         System.out.println("  <main class> args              Runs Java <main class> located on Accumulo classpath");
    +    System.out.println("                                 Add jars to the classpath using '-add jar1:jar2'");
    --- End diff --
   
    I don't think the java code should be documenting the wrapper script, which may or may not be used.
    I also think this feature can be removed, since it's useless.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

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

    https://github.com/apache/accumulo/pull/230#discussion_r104785603
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    --- End diff --
   
    Why not get rid of this `-add` thingy also? It doesn't seem like it's useful if you can set CLASSPATH in the env script.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

joshelser
In reply to this post by joshelser
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/230#discussion_r104792661
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    --- End diff --
   
    What about the case of running a program once and not wanting to change the env script?


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

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

    https://github.com/apache/accumulo/pull/230#discussion_r104793591
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    --- End diff --
   
    It's nice to be able to add a jar quickly to the classpath without modifying accumulo-env.sh like below:
   
    ```bash
    accumulo -add /path/to/my.jar com.my.class
    ```
   
    But I guess users could just do the following instead (after your PR is merged)
   
    ```bash
    CLASSPATH=/path/to/my.jar; accumulo com.my.class
    ```
   
    In the end, I am ok removing the `-add` and `jar` command if I can rename the current `accumulo classpath` command to be `classpath-info` and create a `accumulo classpath` that just prints the classpath created from accumulo-env.sh so the following works:
   
    ```bash
    java -cp /path/to/my.jar:$(accumulo classpath) com.my.class
    ```


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

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

    https://github.com/apache/accumulo/pull/230#discussion_r104796382
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    --- End diff --
   
    Users can already add items with `export CLASSPATH=...` before running the existing scripts, without editing any files. This isn't new.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

joshelser
In reply to this post by joshelser
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/230#discussion_r104796515
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    -    export ACCUMULO_XTRAJARS="$2"
    +    CLASSPATH="${2}:${CLASSPATH}"
         shift 2
    -  else
    -    export ACCUMULO_XTRAJARS=""
       fi
       if [[ "$cmd" = "jar" && -f "$2" ]]; then
    -    if [[ $2 =~ ^/ ]]; then
    -      jardir="$(dirname "$2")"
    -      jarfile="$2"
    -    else
    -      jardir="$(pwd)"
    -      jarfile="${jardir}/${2}"
    -    fi
    -    if jar tf "$jarfile" | grep -q META-INF/MANIFEST.MF ; then
    -      cp="$(unzip -p "$jarfile" META-INF/MANIFEST.MF | grep ^Class-Path: | sed 's/^Class-Path: *//')"
    --- End diff --
   
    https://issues.apache.org/jira/browse/ACCUMULO-1494



---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

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

    https://github.com/apache/accumulo/pull/230#discussion_r104797448
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    -    export ACCUMULO_XTRAJARS="$2"
    +    CLASSPATH="${2}:${CLASSPATH}"
         shift 2
    -  else
    -    export ACCUMULO_XTRAJARS=""
       fi
       if [[ "$cmd" = "jar" && -f "$2" ]]; then
    -    if [[ $2 =~ ^/ ]]; then
    -      jardir="$(dirname "$2")"
    -      jarfile="$2"
    -    else
    -      jardir="$(pwd)"
    -      jarfile="${jardir}/${2}"
    -    fi
    -    if jar tf "$jarfile" | grep -q META-INF/MANIFEST.MF ; then
    -      cp="$(unzip -p "$jarfile" META-INF/MANIFEST.MF | grep ^Class-Path: | sed 's/^Class-Path: *//')"
    --- End diff --
   
    @mikewalch  has previously suggested that we should easily expose a classpath which a user can use to launch their own processes. This makes a lot more sense to me than baking this into our own launch scripts.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

joshelser
In reply to this post by joshelser
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/230#discussion_r104800443
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    -    export ACCUMULO_XTRAJARS="$2"
    +    CLASSPATH="${2}:${CLASSPATH}"
         shift 2
    -  else
    -    export ACCUMULO_XTRAJARS=""
       fi
       if [[ "$cmd" = "jar" && -f "$2" ]]; then
    -    if [[ $2 =~ ^/ ]]; then
    -      jardir="$(dirname "$2")"
    -      jarfile="$2"
    -    else
    -      jardir="$(pwd)"
    -      jarfile="${jardir}/${2}"
    -    fi
    -    if jar tf "$jarfile" | grep -q META-INF/MANIFEST.MF ; then
    -      cp="$(unzip -p "$jarfile" META-INF/MANIFEST.MF | grep ^Class-Path: | sed 's/^Class-Path: *//')"
    --- End diff --
   
    I took a quick look at ACCUMULO-1494 and I think one intention of that change was to allow running a main class specified in a jar.
   
    > @mikewalch has previously suggested that we should easily expose a classpath which a user can use to launch their own processes.
   
    I like that approach.  If we make it easy to construct a classpath, then a user can use `java -jar` to run a main class from a manifest file?  So it seems like the same functionality can be achieved in a more standard way?  I think this would all work, but not completely sure.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

joshelser
In reply to this post by joshelser
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/230#discussion_r104800761
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    -    export ACCUMULO_XTRAJARS="$2"
    +    CLASSPATH="${2}:${CLASSPATH}"
         shift 2
    -  else
    -    export ACCUMULO_XTRAJARS=""
       fi
       if [[ "$cmd" = "jar" && -f "$2" ]]; then
    --- End diff --
   
    If removing, It would be useful to look at the patch in ACCUMULO-1494.  There may be more that needs to be removed.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

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

    https://github.com/apache/accumulo/pull/230#discussion_r104801111
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    --- End diff --
   
    We can add a separate command, but rather than that, I think we should just improve the output of our existing classpath command, so it's in a more usable format, with flags to toggle the classloader hierarchies to include, and with a sensible default.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

joshelser
In reply to this post by joshelser
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/230#discussion_r104803563
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    --- End diff --
   
    I like the idea of adding options to the classpath command


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

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

    https://github.com/apache/accumulo/pull/230#discussion_r104950540
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    -    export ACCUMULO_XTRAJARS="$2"
    +    CLASSPATH="${2}:${CLASSPATH}"
         shift 2
    -  else
    -    export ACCUMULO_XTRAJARS=""
       fi
       if [[ "$cmd" = "jar" && -f "$2" ]]; then
    --- End diff --
   
    I removed the `jar` command in a66cb2888454e9994


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

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

    https://github.com/apache/accumulo/pull/230#discussion_r104952630
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    --- End diff --
   
    I removed the `-add` option in a66cb2888454e9994 and documented how users can set the `CLASSPATH` variable before calling accumulo command.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #230: Accumulo-4599 Simplify how accumulo script adds ...

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

    https://github.com/apache/accumulo/pull/230#discussion_r104965493
 
    --- Diff: assemble/bin/accumulo ---
    @@ -59,43 +59,22 @@ function main() {
       export ACCUMULO_CONF_DIR="$conf"
     
       # Verify setting in accumulo-env.sh
    -
       : "${JAVA_OPTS:?"variable is not set in accumulo-env.sh"}"
       : "${ACCUMULO_LOG_DIR:?"variable is not set in accumulo-env.sh"}"
       mkdir -p "${ACCUMULO_LOG_DIR}" 2>/dev/null
    -  verify_env_dir "ACCUMULO_LOG_DIR" "${ACCUMULO_LOG_DIR}"
       verify_env_dir "HADOOP_PREFIX" "${HADOOP_PREFIX}"
       verify_env_dir "HADOOP_CONF_DIR" "${HADOOP_CONF_DIR}"
       verify_env_dir "ZOOKEEPER_HOME" "${ZOOKEEPER_HOME}"
       : "${MALLOC_ARENA_MAX:?"variable is not set in accumulo-env.sh"}"
     
    -  # ACCUMULO_XTRAJARS is where all of the commandline -add items go into for reading by accumulo.
    -  # It also holds the JAR run with the jar command and, if possible, any items in the JAR manifest's Class-Path.
    +  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
    +
       if [[ "$cmd" = "-add" ]]; then
    --- End diff --
   
    Create new JIRA issue for modifying `accumulo classpath` command: https://issues.apache.org/jira/browse/ACCUMULO-4604


---
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.
---
12
Loading...