[
https://issues.apache.org/jira/browse/DRILL-4581?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul Rogers updated DRILL-4581:
-------------------------------
Description:
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 ...
was:
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"
> 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=/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 ...
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
