Mailing-List: contact issues-help@drill.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@drill.apache.org
Date: Thu, 14 Apr 2016 17:43:25 +0000 (UTC)
From: "ASF GitHub Bot (JIRA)" <jira@apache.org>
To: issues@drill.apache.org
Message-ID: <JIRA.12956312.1459894985000.232588.1460655805505@Atlassian.JIRA>
In-Reply-To: <JIRA.12956312.1459894985000@Atlassian.JIRA>
References: <JIRA.12956312.1459894985000@Atlassian.JIRA>
 <JIRA.12956312.1459894985851@arcas>
Subject: [jira] [Commented] (DRILL-4581) Various problems in the Drill
 startup scripts
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable


    [ https://issues.apache.org/jira/browse/DRILL-4581?page=3Dcom.atlassian=
.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=3D1524=
1578#comment-15241578 ]=20

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

GitHub user paul-rogers opened a pull request:

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

    DRILL-4581: Minor fixes to drill startup scripts for linux

    This pull request replaces a previous one. No code differences; this on=
e resolves some git issues.
   =20
    Provides fixes to a number of small problems as explained in DRILL-4581=
. This work is in preparation for Drill-on-YARN, and contains a new drillbi=
t.sh option to launch Drill under YARN. Otherwise, the public interface to =
the scripts remains unchanged. Features that used to work still work. Featu=
res that were broken now work as originally intended.
   =20
    Does not contain changes for the proposed =E2=80=9Csite=E2=80=9D config=
 extension.

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

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

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

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

    Merge remote-tracking branch 'apache/master'

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

    Merge remote-tracking branch 'apache/master'

commit 3289e5840d54859b5d364101e1a8ae09d135a717
Author: Paul Rogers <progers@maprtech.com>
Date:   2016-04-14T17:35:09Z

    DRILL-4581: Multiple fixes to Drill launch scripts
   =20
    Fixes to a number of small problems as explained in DRILL-4581. This
    work is in preparation for Drill-on-YARN, and contains a new
    drillbit.sh option to launch Drill under YARN. Otherwise, the public
    interface to the scripts remains unchanged. Features that used to work
    still work. Features that were broken now work as originally intended.
    Does not contain changes for the proposed =E2=80=9Csite=E2=80=9D config=
 extension.

----


> Various problems in the Drill startup scripts
> ---------------------------------------------
>
>                 Key: DRILL-4581
>                 URL: https://issues.apache.org/jira/browse/DRILL-4581
>             Project: Apache Drill
>          Issue Type: Bug
>          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=3D/var/log/drill or, if it does not exist, $DRILL_HOM=
E/log
> 2) Comment: DRILL_PID_DIR    The pid files are stored. /tmp by default.
> Code: DRILL_PID_DIR=3D$DRILL_HOME
> 3) Redundant checking of JAVA_HOME. drillbit.sh sources drill-config.sh w=
hich checks JAVA_HOME. Later, drillbit.sh checks it again. The second check=
 is both unnecessary and prints a less informative message than the drill-c=
onfig.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 a=
nd 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 d=
oes not handle the case where that directory is not writable. Suggested: re=
move the check in drillbit.sh.
> 6) Drill-config.sh checks the writability of the DRILL_LOG_DIR by touchin=
g 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 fallb=
ack case with somewhat awkward logic. Clean this up.
> 8) drillbit.sh, but not drill-config.sh, attempts to create /var/log/dril=
l if it does not exist. Recommended: decide on a single choice, implement i=
t in drill-config.sh.
> 9) drill-config.sh checks if $DRILL_CONF_DIR is a directory. If not, defa=
ults 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 o=
f 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 le=
ft over from the original Hadoop script that the Drill script was based upo=
n. Recomendation: export only in the case that HADOOP_HOME is set for cygwi=
n.
> 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 pr=
ints a different message (to stdout) if the version is wrong. Recommendatio=
n: 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 verifie=
s that it is executable. The script then throws away what we just found. Th=
en, drill-bit.sh tries to recreate this information as:
> JAVA=3D$JAVA_HOME/bin/java
> This is wrong in two ways: 1) it ignores the actual java location and ass=
umes it, and 2) it does not handle the java.exe case that drill-config.sh c=
arefully worked out.
> Recommendation: export JAVA from drill-config.sh and remove the above lin=
e from drillbit.sh.
> 14) drillbit.sh presumably takes extra arguments like this:
> drillbit.sh -Dvar0=3Dvalue0 --config /my/conf/dir start -Dvar1=3Dvalue1 -=
Dvar2=3Dvalue2 -Dvar3=3Dvalue3
> The -D bit allows the user to override config variables at the command li=
ne. But, the scripts don't use the values.
> A) drill-config.sh consumes --config /my/conf/dir after consuming the lea=
ding arguments:
> while [ $# -gt 1 ]; do
>   if [ "--config" =3D "$1" ]; then
>     shift
>     confdir=3D$1
>     shift
>     DRILL_CONF_DIR=3D$confdir
>   else
>     # Presume we are at end of options and break
>     break
>   fi
> done
> B) drill-bit.sh will discard the var1:
> startStopStatus=3D$1 <-- grabs "start"
> shift
> command=3Ddrillbit
> shift   <-- Consumes -Dvar1=3Dvalue1
> C) Remaining values passed back into drillbit.sh:
> args=3D$@
> nohup $thiscmd internal_start $command $args
> D) Second invocation discards -Dvar2=3Dvalue2 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=3D$DRILLBIT_LOG_PATH -Dlog.query.path=3D$DRILLBIT_Q=
UERY_LOG_PATH $DRILL_ALL_JAVA_OPTS -cp $CP org.apache.drill.exec.server.Dri=
llbit
> 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 n=
o-argument check should be done AFTER consuming --config.
> 16) As noted above, both drillbit.sh and runbit source drill-config.sh. B=
ut runbit is (apparently) only every called from drillbit.sh. Therefore, we=
 do the same setup & check tasks twice. (In addition to reading --config tw=
ice as noted above.) Recommendation: omit the sourcing of drill-config.sh i=
n runbit.
> 17) The name of the drillbit.sh script is used in many messages. It is ha=
rdcoded, but appears to originally have been taken from $0:
> command=3Ddrillbit
> 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 Drill=
bit. 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 p=
rocess 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=3D"$DRILLBIT_JAVA_OPTS $DRILL_JAVA_OPTS $SERVER_GC_OP=
TS"
> Presumably the DRILL_JAVA_OPTS are for all Drill apps (including the clie=
nt) while DRILLBIT_JAVA_OPTS are for the server (drillbit).
> Since later Java options override earlier ones, more specific options (fo=
r the server) should come after more general ones (for all of Drill). So, o=
rder should be:
> DRILL_ALL_JAVA_OPTS=3D"$DRILL_JAVA_OPTS $DRILLBIT_JAVA_OPTS $SERVER_GC_OP=
TS"
> 21) The startup scripts go to great lengths to properly set up the logs. =
But, we allow drill-env.sh to override them.=20
> exec $JAVA -Dlog.path=3D$DRILLBIT_LOG_PATH -Dlog.query.path=3D$DRILLBIT_Q=
UERY_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 lo=
g 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=3D$DRILLBIT_LOG_PATH -Dlog.qu=
ery.path=3D$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, runbi=
t does not use these arguments. But, if it did, it would get unncesssary cl=
utter.
> 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)