ctubbsii commented on code in PR #2324:
URL: https://github.com/apache/zookeeper/pull/2324#discussion_r2535956836
##########
bin/zkCleanup.sh:
##########
@@ -31,27 +31,44 @@
# use POSIX interface, symlink is followed automatically
ZOOBIN="${BASH_SOURCE-$0}"
ZOOBIN="$(dirname "$ZOOBIN")"
-ZOOBINDIR="$(cd "$ZOOBIN" && pwd)"
+ZOOBINDIR="$(cd "$ZOOBIN" || exit && pwd)"
Review Comment:
Will this exist the entire script, or just the subshell that the `cd`
command is running in?
It might be better to do something like:
```suggestion
ZOOBINDIR="$(cd "$ZOOBIN" && pwd)" || exit
```
This is a little more clear what is going to happen, especially when it
comes to the order of operations between `||` and `&&`, which are not
immediately obvious to non-bash experts.
##########
bin/zkCleanup.sh:
##########
@@ -31,27 +31,44 @@
# use POSIX interface, symlink is followed automatically
ZOOBIN="${BASH_SOURCE-$0}"
ZOOBIN="$(dirname "$ZOOBIN")"
-ZOOBINDIR="$(cd "$ZOOBIN" && pwd)"
+ZOOBINDIR="$(cd "$ZOOBIN" || exit && pwd)"
if [[ -e "$ZOOBIN/../libexec/zkEnv.sh" ]]; then
- # shellcheck source=bin/zkEnv.sh
Review Comment:
Why remove the shellcheck hints? Those help shellcheck find the scripts in
the source code when doing validation.
##########
bin/zkCleanup.sh:
##########
@@ -31,27 +31,44 @@
# use POSIX interface, symlink is followed automatically
ZOOBIN="${BASH_SOURCE-$0}"
ZOOBIN="$(dirname "$ZOOBIN")"
-ZOOBINDIR="$(cd "$ZOOBIN" && pwd)"
+ZOOBINDIR="$(cd "$ZOOBIN" || exit && pwd)"
if [[ -e "$ZOOBIN/../libexec/zkEnv.sh" ]]; then
- # shellcheck source=bin/zkEnv.sh
- . "$ZOOBINDIR"/../libexec/zkEnv.sh
+ . "$ZOOBINDIR"/../libexec/zkEnv.sh "$@"
else
- # shellcheck source=bin/zkEnv.sh
- . "$ZOOBINDIR"/zkEnv.sh
+ . "$ZOOBINDIR"/zkEnv.sh "$@"
fi
-ZOODATADIR="$(grep "^[[:space:]]*dataDir=" "$ZOOCFG" | sed -e 's/.*=//')"
-ZOODATALOGDIR="$(grep "^[[:space:]]*dataLogDir=" "$ZOOCFG" | sed -e 's/.*=//')"
+ZOODATADIR=""
+ZOODATALOGDIR=""
+
+# Only try to read config if ZOOCFG exists
+if [[ -f $ZOOCFG ]]; then
+ ZOODATADIR="$(grep "^[[:space:]]*dataDir=" "$ZOOCFG" 2>/dev/null | sed -e
's/.*=//')"
+ ZOODATALOGDIR="$(grep "^[[:space:]]*dataLogDir=" "$ZOOCFG" 2>/dev/null | sed
-e 's/.*=//')"
+fi
ZOO_LOG_FILE=zookeeper-$USER-cleanup-$HOSTNAME.log
# shellcheck disable=SC2206
flags=($JVMFLAGS)
-if [[ -z $ZOODATALOGDIR ]]; then
- "$JAVA" "-Dzookeeper.log.dir=$ZOO_LOG_DIR"
"-Dzookeeper.log.file=$ZOO_LOG_FILE" "${flags[@]}" \
- org.apache.zookeeper.server.PurgeTxnLog "$ZOODATADIR" "$@"
+
+# If config provides directories, use them; otherwise pass all args to
PurgeTxnLog
+if [[ -n $ZOODATADIR ]]; then
+ if [[ -z $ZOODATALOGDIR ]]; then
+ # Only dataDir specified
+ "$JAVA" "-Dzookeeper.log.dir=$ZOO_LOG_DIR"
"-Dzookeeper.log.file=$ZOO_LOG_FILE" \
+ -cp "$CLASSPATH" "${flags[@]}" \
Review Comment:
The CLASSPATH environment variable is already exported. Passing it again as
`-cp $CLASSPATH` is redundant, and makes really really long output in `ps`, and
sometimes breaks the ability to use `pkill` or `pgrep` on the process, due to
the length of the command-line. It is enough to export the variable. You don't
need `-cp`.
(Same comment applies in the other cases where you've added it, as well.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
