[ 
https://issues.apache.org/jira/browse/DRILL-4581?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15373920#comment-15373920
 ] 

ASF GitHub Bot commented on DRILL-4581:
---------------------------------------

GitHub user paul-rogers opened a pull request:

    https://github.com/apache/drill/pull/544

    DRILL 4581 Revised

    Extensive revisions to the Drill scripts to prepare them for
    Drill-on-YARN, to provide a full site directory, to move
    distribution-specific settings out of the user’s drill-env.sh file, and
    to fix the bugs outlined in DRILL-4581.
    
    Contains only Drill script fixes; does not contain the new
    Drill-on-YARN scripts.

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

    $ git pull https://github.com/paul-rogers/drill DRILL-4581-Revised

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

    https://github.com/apache/drill/pull/544.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 #544
    
----
commit abbfe84e35517c37f59a507694a4f0224137d2b8
Author: Paul Rogers <prog...@maprtech.com>
Date:   2016-04-08T21:04:40Z

    Merge remote-tracking branch 'apache/master'

commit e68ab2d8c50ce4d64631f410cbcbe7f4e98aeb8c
Author: Paul Rogers <prog...@maprtech.com>
Date:   2016-04-13T23:27:06Z

    Merge remote-tracking branch 'apache/master'

commit ce7d43be1808690f80f6a3c0a826ffb2725ef817
Author: Paul Rogers <prog...@maprtech.com>
Date:   2016-07-11T02:55:09Z

    Merge remote-tracking branch 'apache/master'

commit 291f28219fa11ab317ac4b0923e68502881c0b07
Author: Paul Rogers <prog...@maprtech.com>
Date:   2016-07-12T23:19:28Z

    DRILL-4581 - Drill script fixes
    
    Extensive revisions to the Drill scripts to prepare them for
    Drill-on-YARN, to provide a full site directory, to move
    distribution-specific settings out of the user’s drill-env.sh file, and
    to fix the bugs outlined in DRILL-4581.
    
    Contains only Drill script fixes; does not contain the new
    Drill-on-YARN scripts.

----


> Various problems in the Drill startup scripts
> ---------------------------------------------
>
>                 Key: DRILL-4581
>                 URL: https://issues.apache.org/jira/browse/DRILL-4581
>             Project: Apache Drill
>          Issue Type: Sub-task
>          Components:  Server
>    Affects Versions: 1.6.0
>            Reporter: Paul Rogers
>            Assignee: Paul Rogers
>            Priority: Minor
>
> Noticed the following in drillbit.sh:
> 1) Comment: DRILL_LOG_DIR    Where log files are stored.  PWD by default.
> Code: DRILL_LOG_DIR=/var/log/drill or, if it does not exist, $DRILL_HOME/log
> 2) Comment: DRILL_PID_DIR    The pid files are stored. /tmp by default.
> Code: DRILL_PID_DIR=$DRILL_HOME
> 3) Redundant checking of JAVA_HOME. drillbit.sh sources drill-config.sh which 
> checks JAVA_HOME. Later, drillbit.sh checks it again. The second check is 
> both unnecessary and prints a less informative message than the 
> drill-config.sh check. Suggestion: Remove the JAVA_HOME check in drillbit.sh.
> 4) Though drill-config.sh carefully checks JAVA_HOME, it does not export the 
> JAVA_HOME variable. Perhaps this is why drillbit.sh repeats the check? 
> Recommended: export JAVA_HOME from drill-config.sh.
> 5) Both drillbit.sh and the sourced drill-config.sh check DRILL_LOG_DIR and 
> set the default value. Drill-config.sh defaults to /var/log/drill, or if that 
> fails, to $DRILL_HOME/log. Drillbit.sh just sets /var/log/drill and does not 
> handle the case where that directory is not writable. Suggested: remove the 
> check in drillbit.sh.
> 6) Drill-config.sh checks the writability of the DRILL_LOG_DIR by touching 
> sqlline.log, but does not delete that file, leaving a bogus, empty client log 
> file on the drillbit server. Recommendation: use bash commands instead.
> 7) The implementation of the above check is a bit awkward. It has a fallback 
> case with somewhat awkward logic. Clean this up.
> 8) drillbit.sh, but not drill-config.sh, attempts to create /var/log/drill if 
> it does not exist. Recommended: decide on a single choice, implement it in 
> drill-config.sh.
> 9) drill-config.sh checks if $DRILL_CONF_DIR is a directory. If not, defaults 
> it to $DRILL_HOME/conf. This can lead to subtle errors. If I use
> drillbit.sh --config /misspelled/path
> where I mistype the path, I won't get an error, I get the default config, 
> which may not at all be what I want to run. Recommendation: if the value of 
> DRILL_CONF_DRILL is passed into the script (as a variable or via --config), 
> then that directory must exist. Else, use the default.
> 10) drill-config.sh exports, but may not set, HADOOP_HOME. This may be left 
> over from the original Hadoop script that the Drill script was based upon. 
> Recomendation: export only in the case that HADOOP_HOME is set for cygwin.
> 11) Drill-config.sh checks JAVA_HOME and prints a big, bold error message to 
> stderr if JAVA_HOME is not set. Then, it checks the Java version and prints a 
> different message (to stdout) if the version is wrong. Recommendation: use 
> the same format (and stderr) for both.
> 12) Similarly, other Java checks later in the script produce messages to 
> stdout, not stderr.
> 13) Drill-config.sh searches $JAVA_HOME to find java/java.exe and verifies 
> that it is executable. The script then throws away what we just found. Then, 
> drill-bit.sh tries to recreate this information as:
> JAVA=$JAVA_HOME/bin/java
> This is wrong in two ways: 1) it ignores the actual java location and assumes 
> it, and 2) it does not handle the java.exe case that drill-config.sh 
> carefully worked out.
> Recommendation: export JAVA from drill-config.sh and remove the above line 
> from drillbit.sh.
> 14) drillbit.sh presumably takes extra arguments like this:
> drillbit.sh -Dvar0=value0 --config /my/conf/dir start -Dvar1=value1 
> -Dvar2=value2 -Dvar3=value3
> The -D bit allows the user to override config variables at the command line. 
> But, the scripts don't use the values.
> A) drill-config.sh consumes --config /my/conf/dir after consuming the leading 
> arguments:
> while [ $# -gt 1 ]; do
>   if [ "--config" = "$1" ]; then
>     shift
>     confdir=$1
>     shift
>     DRILL_CONF_DIR=$confdir
>   else
>     # Presume we are at end of options and break
>     break
>   fi
> done
> B) drill-bit.sh will discard the var1:
> startStopStatus=$1 <-- grabs "start"
> shift
> command=drillbit
> shift   <-- Consumes -Dvar1=value1
> C) Remaining values passed back into drillbit.sh:
> args=$@
> nohup $thiscmd internal_start $command $args
> D) Second invocation discards -Dvar2=value2 as described above.
> E) Remaining values are passed to runbit:
> "$DRILL_HOME"/bin/runbit  $command "$@" start
> F) Where they again pass though drill-config. (Allowing us to do:
> drillbit.sh --config /first/conf --config /second/conf
> which is asking for trouble)
> G) And, the remaining arguments are simply not used:
> exec $JAVA -Dlog.path=$DRILLBIT_LOG_PATH 
> -Dlog.query.path=$DRILLBIT_QUERY_LOG_PATH $DRILL_ALL_JAVA_OPTS -cp $CP 
> org.apache.drill.exec.server.Drillbit
> 15) The checking of command-line args in drillbit.sh is wrong:
> # if no args specified, show usage
> if [ $# -lt 1 ]; then
>   echo $usage
>   exit 1
> fi
> ...
> . "$bin"/drill-config.sh
> But, note, that drill-config.sh handles:
> drillbit.sh --config /conf/dir
> Consuming those two arguments will leave no command argument. Thus, the 
> no-argument check should be done AFTER consuming --config.
> 16) As noted above, both drillbit.sh and runbit source drill-config.sh. But 
> runbit is (apparently) only every called from drillbit.sh. Therefore, we do 
> the same setup & check tasks twice. (In addition to reading --config twice as 
> noted above.) Recommendation: omit the sourcing of drill-config.sh in runbit.
> 17) The name of the drillbit.sh script is used in many messages. It is 
> hardcoded, but appears to originally have been taken from $0:
> command=drillbit
> shift
> Recommended: get the name from the script, strip the directory, and strip the 
> suffix. So, /foo/drillbit2.sh becomes drillbit2, \windir\drillbit2.bat 
> becomes drillbit2.
> 18) drillbit.sh creates a pid file to record the pid of the running Drillbit. 
> However, the file is not deleted upon normal Drill exit. Better would be to 
> remove this file on exit to keep things tidy.
> 19) Similarly, when the stop command detects a pid file, but no running 
> process with that pid, it prints a message saying so, but does not clean up 
> the (unwanted) pid file. It should do so.
> 20) The runbit script sets up Java options as follows:
> DRILL_ALL_JAVA_OPTS="$DRILLBIT_JAVA_OPTS $DRILL_JAVA_OPTS $SERVER_GC_OPTS"
> Presumably the DRILL_JAVA_OPTS are for all Drill apps (including the client) 
> while DRILLBIT_JAVA_OPTS are for the server (drillbit).
> Since later Java options override earlier ones, more specific options (for 
> the server) should come after more general ones (for all of Drill). So, order 
> should be:
> DRILL_ALL_JAVA_OPTS="$DRILL_JAVA_OPTS $DRILLBIT_JAVA_OPTS $SERVER_GC_OPTS"
> 21) The startup scripts go to great lengths to properly set up the logs. But, 
> we allow drill-env.sh to override them. 
> exec $JAVA -Dlog.path=$DRILLBIT_LOG_PATH 
> -Dlog.query.path=$DRILLBIT_QUERY_LOG_PATH $DRILL_ALL_JAVA_OPTS ...
> The computed log properties come before the user-defined properties, and so 
> user defined properties can override those log settings. The computed log 
> settings are used to write log entries and should be considered "correct."
> Recommended: change option order as follows:
> exec $JAVA  $DRILL_ALL_JAVA_OPTS -Dlog.path=$DRILLBIT_LOG_PATH 
> -Dlog.query.path=$DRILLBIT_QUERY_LOG_PATH ...
> 22) As above, but in sqlline:
> exec "$JAVA" $DRILL_SHELL_JAVA_OPTS $DRILL_JAVA_OPTS
> Should be:
> exec "$JAVA"  $DRILL_JAVA_OPTS $DRILL_SHELL_JAVA_OPTS
> 23) The implementations of each command unnecessarily pass along $command on 
> internal calls:
> (start)
>     nohup $thiscmd internal_start $command $args < /dev/null >> ${logout} 
> 2>&1  &
> (internal_start)
>     nice -n $DRILL_NICENESS "$DRILL_HOME"/bin/runbit \
>         $command "$@" start >> "$logout" 2>&1 &
> Results in "drillbit drillbit start" being passed to runbit. Today, runbit 
> does not use these arguments. But, if it did, it would get unncesssary 
> clutter.
> 24) Benign. The restart command passes $command uncessarily when calling the 
> drillbit.sh script recursively:
>     $thiscmd --config "${DRILL_CONF_DIR}" stop $command $args &
>     $thiscmd --config "${DRILL_CONF_DIR}" start $command $args &
> results in drillbit.sh --conf ... stop drill bit
> 25) The stop command removes a file using an undefined variable:
>     rm -f "$DRILL_START_FILE"
> This variable is never defined in any of the startup scripts. If the user can 
> define it, we should be checking if the variable is empty. Suggestion: remove 
> this line.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to