[GitHub] accumulo pull request #218: ACCUMULO-4528 Accumulo scripts improvements

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

[GitHub] accumulo pull request #218: ACCUMULO-4528 Accumulo scripts improvements

ctubbsii
GitHub user mikewalch opened a pull request:

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

    ACCUMULO-4528 Accumulo scripts improvements

    * Accumulo now determines hostname in Java
    * Minimized use of environment variables
    * Consolidated scripts by moving service.sh and cluster.sh into
      accumulo-service and accumulo-cluster which were moved to contrib/.
      Also, moved masters, tservers, tracers, etc files to contrib/conf/
    * Removed accumulo-watcher script as restarting services should not be
      handled by Accumulo scripts
    * NUMA and multiple tservers are no longer configured in scripts but
      could be NUMA commands could be added using ACCUMULO_JAVA_PREFIX env
      variable.
    * Renamed accumulo-env.sh to accumulo.conf
    * Moved create-config and build-native from accumulo script to
      accumulo-config and accumulo-native scripts in contrib/

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

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

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

    https://github.com/apache/accumulo/pull/218.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 #218
   
----

----


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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

    https://github.com/apache/accumulo/pull/218#discussion_r101361758
 
    --- Diff: INSTALL.md ---
    @@ -57,15 +57,15 @@ The script will ask you questions about your set up. Below are some suggestions:
       processes like Hadoop, Zookeeper, and the Accumulo client code.  If Accumulo
       worker processes are swapped out and unresponsive, they may be killed.
     
    -After the `create-config` command is run, the `conf/` directory will contain
    -`accumulo-env.sh`, `accumulo-site.xml`, and few a additional files. These files require
    +After the `accumulo-config` command is run, the `conf/` directory will contain
    +`accumulo.conf`, `accumulo-site.xml`, and few a additional files. These files require
     a few edits before starting Accumulo.
     
     ### Secret
     
     Accumulo coordination and worker processes can only communicate with each other
     if they share the same secret key.  To change the secret key set
    -`instance.secret` in `conf/accumulo-site.xml`.  Changing this secret key from
    +`instance.secret` in `accumulo-site.xml`.  Changing this secret key from
    --- End diff --
   
    Did you want to move accumulo-site.xml out of conf?


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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

    https://github.com/apache/accumulo/pull/218#discussion_r101365500
 
    --- Diff: INSTALL.md ---
    @@ -57,15 +57,15 @@ The script will ask you questions about your set up. Below are some suggestions:
       processes like Hadoop, Zookeeper, and the Accumulo client code.  If Accumulo
       worker processes are swapped out and unresponsive, they may be killed.
     
    -After the `create-config` command is run, the `conf/` directory will contain
    -`accumulo-env.sh`, `accumulo-site.xml`, and few a additional files. These files require
    +After the `accumulo-config` command is run, the `conf/` directory will contain
    +`accumulo.conf`, `accumulo-site.xml`, and few a additional files. These files require
     a few edits before starting Accumulo.
     
     ### Secret
     
     Accumulo coordination and worker processes can only communicate with each other
     if they share the same secret key.  To change the secret key set
    -`instance.secret` in `conf/accumulo-site.xml`.  Changing this secret key from
    +`instance.secret` in `accumulo-site.xml`.  Changing this secret key from
    --- End diff --
   
    It's still located in conf for the tarball distribution but I changed the manual to not include directory/path of the file as accumulo-site.xml could be in other locations like `/etc/accumulo` in downstream packaging.


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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/218#discussion_r101373614
 
    --- Diff: assemble/contrib/tool.sh ---
    @@ -26,14 +26,21 @@ contrib=$( cd -P "$( dirname "$SOURCE" )" && pwd )
     basedir=$( cd -P "${contrib}"/.. && pwd )
     # Stop: Resolve Script Directory
     
    -source "$basedir"/libexec/load-env.sh
    +conf="${basedir}/conf"
    +if [ ! -f "${conf}/accumulo.conf" ]; then
    +  echo "accumulo.conf must exist in $conf"
    +  echo "Run 'accumulo-config' to create it or copy it from $conf/examples"
    +  echo "Follow the instructions in INSTALL.md to edit it for your environment."
    +  exit 1
    +fi
    +source "$conf/accumulo.conf"  
     
     if [[ -z "$HADOOP_PREFIX" ]] ; then
    -   echo "HADOOP_PREFIX is not set.  Please make sure it's set globally or in conf/accumulo-env.sh"
    +   echo "HADOOP_PREFIX is not set.  Please make sure it's set globally or in $conf/accumulo.conf"
    --- End diff --
   
    I think this is currently necessary for the property interpolation in the `accumulo-site.xml` file, but I'd really like to phase out having it as a requirement.


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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/218#discussion_r101365517
 
    --- Diff: assemble/conf/examples/generic_logger.properties ---
    @@ -24,7 +24,7 @@ log4j.appender.A2.layout.ConversionPattern=%d{ISO8601} [%-8c{2}] %-5p: %m%n
     
     # Write out INFO and higher to the regular log
     log4j.appender.A3=org.apache.log4j.RollingFileAppender
    -log4j.appender.A3.File=${org.apache.accumulo.core.dir.log}/${org.apache.accumulo.core.application}_${accumulo.service.instance}${org.apache.accumulo.core.ip.localhost.hostname}_fromprops.log
    +log4j.appender.A3.File=${org.apache.accumulo.core.dir.log}/${org.apache.accumulo.core.application}${accumulo.service.instance}_${org.apache.accumulo.core.ip.localhost.hostname}_fromprops.log
    --- End diff --
   
    Maybe we shouldn't enable the rolling file appender in the default configs. Users can add that if they want to. A better option would probably be to use the syslog appender by 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
|

[GitHub] accumulo pull request #218: ACCUMULO-4528 Accumulo scripts improvements

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/218#discussion_r101363077
 
    --- Diff: INSTALL.md ---
    @@ -40,13 +40,13 @@ Accumulo has some optional native code that improves its performance and
     stability. Before configuring Accumulo, attempt to build this native code
     with the following command.
     
    -    accumulo build-native
    +    ./contrib/accumulo-native
    --- End diff --
   
    I'm not sure contrib is the best place for this. I think of contrib items as stuff that adds on to, but is unknown to, the core project. The native libraries are something that are optional, but known to Accumulo, and tightly coupled with it. I think of contrib stuff as optional stuff many users can ignore. But, native libraries are something we strongly recommend.


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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/218#discussion_r101363679
 
    --- Diff: INSTALL.md ---
    @@ -57,15 +57,15 @@ The script will ask you questions about your set up. Below are some suggestions:
       processes like Hadoop, Zookeeper, and the Accumulo client code.  If Accumulo
       worker processes are swapped out and unresponsive, they may be killed.
     
    -After the `create-config` command is run, the `conf/` directory will contain
    -`accumulo-env.sh`, `accumulo-site.xml`, and few a additional files. These files require
    +After the `accumulo-config` command is run, the `conf/` directory will contain
    +`accumulo.conf`, `accumulo-site.xml`, and few a additional files. These files require
     a few edits before starting Accumulo.
     
     ### Secret
     
     Accumulo coordination and worker processes can only communicate with each other
     if they share the same secret key.  To change the secret key set
    -`instance.secret` in `conf/accumulo-site.xml`.  Changing this secret key from
    +`instance.secret` in `accumulo-site.xml`.  Changing this secret key from
    --- End diff --
   
    I didn't read it that way. I think the previous paragraph already identified the location of the file. This is just referring to it by name, assuming you already read the part which told you where to find it.


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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/218#discussion_r101365020
 
    --- Diff: assemble/conf/examples/generic_logger.properties ---
    @@ -15,7 +15,7 @@
     
     # Write out everything at the DEBUG level to the debug log
     log4j.appender.A2=org.apache.log4j.RollingFileAppender
    -log4j.appender.A2.File=${org.apache.accumulo.core.dir.log}/${org.apache.accumulo.core.application}_${accumulo.service.instance}${org.apache.accumulo.core.ip.localhost.hostname}_fromprops.debug.log
    +log4j.appender.A2.File=${org.apache.accumulo.core.dir.log}/${org.apache.accumulo.core.application}${accumulo.service.instance}_${org.apache.accumulo.core.ip.localhost.hostname}_fromprops.debug.log
    --- End diff --
   
    Rather than produce these system properties in Accumulo code, we should use the log4j feature to do `${sys:HOSTNAME}` (might be log4j2 only, but that's okay... we should switch to that)


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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/218#discussion_r101373362
 
    --- Diff: assemble/contrib/tool.sh ---
    @@ -26,14 +26,21 @@ contrib=$( cd -P "$( dirname "$SOURCE" )" && pwd )
     basedir=$( cd -P "${contrib}"/.. && pwd )
     # Stop: Resolve Script Directory
     
    -source "$basedir"/libexec/load-env.sh
    +conf="${basedir}/conf"
    +if [ ! -f "${conf}/accumulo.conf" ]; then
    +  echo "accumulo.conf must exist in $conf"
    --- End diff --
   
    Why wouldn't this file be optional?


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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/218#discussion_r101364066
 
    --- Diff: INSTALL.md ---
    @@ -76,19 +76,18 @@ When configuring Accumulo the following information about these dependencies
     must be provided.
     
      * **Location of Zookeepers** :  Provide this by setting `instance.zookeeper.host`
    -   in `conf/accumulo-site.xml`.
    +   in `accumulo-site.xml`.
      * **Where to store data** :  Provide this by setting `instance.volumes` in
    -   `conf/accumulo-site.xml`.  If your namenode is running at 192.168.1.9:9000
    +   `accumulo-site.xml`.  If your namenode is running at 192.168.1.9:9000
        and you want to store data in `/accumulo` in HDFS, then set
       `instance.volumes` to `hdfs://192.168.1.9:9000/accumulo`.
      * **Location of Zoookeeper and Hadoop jars** :  Setting `ZOOKEEPER_HOME` and
    -   `HADOOP_PREFIX` in `conf/accumulo-env.sh` will help Accumulo find these
    -   jars.
    +   `HADOOP_PREFIX` in `accumulo.conf` will help Accumulo find these jars.
    --- End diff --
   
    Using the default config, this is true, but not necessarily otherwise.


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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/218#discussion_r101363305
 
    --- Diff: INSTALL.md ---
    @@ -40,13 +40,13 @@ Accumulo has some optional native code that improves its performance and
     stability. Before configuring Accumulo, attempt to build this native code
     with the following command.
     
    -    accumulo build-native
    +    ./contrib/accumulo-native
     
     If the command fails, its OK to continue with setup and resolve the issue later.
     
     Run the command below to create configuration for Accumulo in `conf/`:
     
    -    accumulo create-config
    +    ./contrib/accumulo-config
    --- End diff --
   
    Is this an optional command? Is it possible to run Accumulo without doing this? If not, then I don't think contrib is the right place.


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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

    https://github.com/apache/accumulo/pull/218#discussion_r101378663
 
    --- Diff: INSTALL.md ---
    @@ -25,11 +25,11 @@ source code.  Unpack as follows.
         tar xzf <some dir>/accumulo-X.Y.Z-bin.tar.gz
         cd accumulo-X.Y.Z
     
    -There are three scripts in the the `bin/` directory that are used to manage Accumulo:
    +There are three scripts in the tarball distribution that are used to manage Accumulo:
     
    -1. `accumulo` - Runs Accumulo command-line tools and starts Accumulo processes
    -2. `accumulo-service` - Runs Accumulo processes as services
    -3. `accumulo-cluster` - Manages Accumulo cluster on a single node or several nodes
    +1. `bin/accumulo` - Runs Accumulo command-line tools and starts Accumulo processes
    +2. `contrib/accumulo-service` - Runs Accumulo processes as services
    --- End diff --
   
    What does it mean for a script to be in contrib vs bin?


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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/218#discussion_r101399584
 
    --- Diff: INSTALL.md ---
    @@ -25,11 +25,11 @@ source code.  Unpack as follows.
         tar xzf <some dir>/accumulo-X.Y.Z-bin.tar.gz
         cd accumulo-X.Y.Z
     
    -There are three scripts in the the `bin/` directory that are used to manage Accumulo:
    +There are three scripts in the tarball distribution that are used to manage Accumulo:
     
    -1. `accumulo` - Runs Accumulo command-line tools and starts Accumulo processes
    -2. `accumulo-service` - Runs Accumulo processes as services
    -3. `accumulo-cluster` - Manages Accumulo cluster on a single node or several nodes
    +1. `bin/accumulo` - Runs Accumulo command-line tools and starts Accumulo processes
    +2. `contrib/accumulo-service` - Runs Accumulo processes as services
    --- End diff --
   
    In my view, `contrib/` should be for add-ons/wrappers which could be separate projects on their own, but which we are currently maintaining to provide convenient functionality.
   
    I think it makes sense to have a basic launch script in `bin/`, which minimally wraps the execution of java. That minimal wrapper wouldn't make sense as its own project and thus (by my opinion) shouldn't be in `contrib/`.
   
    I'm far less opinionated on the convenience scripts for producing the example configs and for executing `make` on the unpacked native tarball. They could probably go either place.


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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

    https://github.com/apache/accumulo/pull/218#discussion_r101530931
 
    --- Diff: INSTALL.md ---
    @@ -25,11 +25,11 @@ source code.  Unpack as follows.
         tar xzf <some dir>/accumulo-X.Y.Z-bin.tar.gz
         cd accumulo-X.Y.Z
     
    -There are three scripts in the the `bin/` directory that are used to manage Accumulo:
    +There are three scripts in the tarball distribution that are used to manage Accumulo:
     
    -1. `accumulo` - Runs Accumulo command-line tools and starts Accumulo processes
    -2. `accumulo-service` - Runs Accumulo processes as services
    -3. `accumulo-cluster` - Manages Accumulo cluster on a single node or several nodes
    +1. `bin/accumulo` - Runs Accumulo command-line tools and starts Accumulo processes
    +2. `contrib/accumulo-service` - Runs Accumulo processes as services
    --- End diff --
   
    I thought about having a contrib and a bin directory. I think there should only be a bin/ directory.  Two directories will confuse people.  We can communicate that scripts are optional/add-on in the documentation rather than the directory structure.  To limit the number of scripts in bin, we can combine many of the scripts in contrib into one or two scripts.  If a script doesn't work or we don't want to support it any more the in the tarball distribution, it should either be removed or move to a contrib directory in the repo that doesn't get added to the distribution. Contrib scripts should not be distributed.  As for the scripts in bin/, downstream packagers can pick and choose what they include.


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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/218#discussion_r101576229
 
    --- Diff: INSTALL.md ---
    @@ -25,11 +25,11 @@ source code.  Unpack as follows.
         tar xzf <some dir>/accumulo-X.Y.Z-bin.tar.gz
         cd accumulo-X.Y.Z
     
    -There are three scripts in the the `bin/` directory that are used to manage Accumulo:
    +There are three scripts in the tarball distribution that are used to manage Accumulo:
     
    -1. `accumulo` - Runs Accumulo command-line tools and starts Accumulo processes
    -2. `accumulo-service` - Runs Accumulo processes as services
    -3. `accumulo-cluster` - Manages Accumulo cluster on a single node or several nodes
    +1. `bin/accumulo` - Runs Accumulo command-line tools and starts Accumulo processes
    +2. `contrib/accumulo-service` - Runs Accumulo processes as services
    --- End diff --
   
    Seems reasonable.


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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

    https://github.com/apache/accumulo/pull/218#discussion_r101582008
 
    --- Diff: INSTALL.md ---
    @@ -57,15 +57,15 @@ The script will ask you questions about your set up. Below are some suggestions:
       processes like Hadoop, Zookeeper, and the Accumulo client code.  If Accumulo
       worker processes are swapped out and unresponsive, they may be killed.
     
    -After the `create-config` command is run, the `conf/` directory will contain
    -`accumulo-env.sh`, `accumulo-site.xml`, and few a additional files. These files require
    +After the `accumulo-config` command is run, the `conf/` directory will contain
    +`accumulo.conf`, `accumulo-site.xml`, and few a additional files. These files require
     a few edits before starting Accumulo.
     
     ### Secret
     
     Accumulo coordination and worker processes can only communicate with each other
     if they share the same secret key.  To change the secret key set
    -`instance.secret` in `conf/accumulo-site.xml`.  Changing this secret key from
    +`instance.secret` in `accumulo-site.xml`.  Changing this secret key from
    --- End diff --
   
    Got it.  I took the changes out of context.


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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

    https://github.com/apache/accumulo/pull/218
 
    +1 On the renaming of accumulo-env.sh to accumulo.conf.  The .conf extension is pretty clear what the file is for, its simply for configuration purposes.  The -env.sh extension tells me this shell script is setting up the environment but could be doing more then configuration, which makes me nervous just seeing the name of the file.  From a user perspective, I would be much more inclined and comfortable to modify a .conf 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 #218: ACCUMULO-4528 Accumulo scripts improvements

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

    https://github.com/apache/accumulo/pull/218#discussion_r101609853
 
    --- Diff: INSTALL.md ---
    @@ -40,13 +40,13 @@ Accumulo has some optional native code that improves its performance and
     stability. Before configuring Accumulo, attempt to build this native code
     with the following command.
     
    -    accumulo build-native
    +    ./contrib/accumulo-native
    --- End diff --
   
    I pushed a commit where I moved the command to `bin/accumulo-util build-native`


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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

    https://github.com/apache/accumulo/pull/218#discussion_r101610417
 
    --- Diff: assemble/contrib/tool.sh ---
    @@ -26,14 +26,21 @@ contrib=$( cd -P "$( dirname "$SOURCE" )" && pwd )
     basedir=$( cd -P "${contrib}"/.. && pwd )
     # Stop: Resolve Script Directory
     
    -source "$basedir"/libexec/load-env.sh
    +conf="${basedir}/conf"
    +if [ ! -f "${conf}/accumulo.conf" ]; then
    +  echo "accumulo.conf must exist in $conf"
    --- End diff --
   
    I made it optional in 7f9da9f12ce5940


---
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 #218: ACCUMULO-4528 Accumulo scripts improvements

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

    https://github.com/apache/accumulo/pull/218#discussion_r101782473
 
    --- Diff: INSTALL.md ---
    @@ -40,13 +40,13 @@ Accumulo has some optional native code that improves its performance and
     stability. Before configuring Accumulo, attempt to build this native code
     with the following command.
     
    -    accumulo build-native
    +    ./contrib/accumulo-native
     
     If the command fails, its OK to continue with setup and resolve the issue later.
     
     Run the command below to create configuration for Accumulo in `conf/`:
     
    -    accumulo create-config
    +    ./contrib/accumulo-config
    --- End diff --
   
    This was moved to `bin/accumulo-util create-config`


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