ctubbsii commented on code in PR #2324:
URL: https://github.com/apache/zookeeper/pull/2324#discussion_r2557632251
##########
bin/zkCleanup.sh:
##########
@@ -31,27 +31,43 @@
# use POSIX interface, symlink is followed automatically
ZOOBIN="${BASH_SOURCE-$0}"
ZOOBIN="$(dirname "$ZOOBIN")"
-ZOOBINDIR="$(cd "$ZOOBIN" && pwd)"
+ZOOBINDIR="$(cd "$ZOOBIN" && pwd)" || exit
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/.*=//')"
Review Comment:
This is perhaps something for another PR, since it's a bit out of scope
here, and there may be other scripts that would also benefit from this change,
but I noticed that `sed` has greedy matching here and doesn't trim the
whitespace in the value. `cut -f2- -d=` could be used instead to avoid the
greedy matching, but still doesn't trim whitespace in the extracted property
value.
It can be done with bash regular expressions and a simple capture group,
though, without calling another process:
```bash
re='^[^=]*=[[:space:]]*(.*)[[:space:]]*$'
ZOODATADIR="$(grep "^[[:space:]]*dataDir=" "$ZOOCFG" 2>/dev/null)"
[[ $ZOODATADIR =~ $re ]] && ZOODATADIR="${BASH_REMATCH[1]}"
ZOODATALOGDIR="$(grep "^[[:space:]]*dataLogDir=" "$ZOOCFG" 2>/dev/null)"
[[ $ZOODATALOGDIR =~ $re ]] && ZOODATALOGDIR="${BASH_REMATCH[1]}"
```
##########
bin/zkCleanup.sh:
##########
@@ -31,27 +31,43 @@
# use POSIX interface, symlink is followed automatically
ZOOBIN="${BASH_SOURCE-$0}"
ZOOBIN="$(dirname "$ZOOBIN")"
-ZOOBINDIR="$(cd "$ZOOBIN" && pwd)"
+ZOOBINDIR="$(cd "$ZOOBIN" && pwd)" || exit
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)
Review Comment:
It looks like `"-Dzookeeper.log.dir=$ZOO_LOG_DIR"
"-Dzookeeper.log.file=$ZOO_LOG_FILE"` is added in all cases. So, it could just
be added to `flags` here and removed from the subsequent calls:
```suggestion
flags=("-Dzookeeper.log.dir=$ZOO_LOG_DIR"
"-Dzookeeper.log.file=$ZOO_LOG_FILE" $JVMFLAGS)
```
##########
bin/zkCleanup.sh:
##########
@@ -31,27 +31,43 @@
# use POSIX interface, symlink is followed automatically
ZOOBIN="${BASH_SOURCE-$0}"
ZOOBIN="$(dirname "$ZOOBIN")"
-ZOOBINDIR="$(cd "$ZOOBIN" && pwd)"
+ZOOBINDIR="$(cd "$ZOOBIN" && pwd)" || exit
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" \
+ "${flags[@]}" org.apache.zookeeper.server.PurgeTxnLog "$ZOODATADIR" "$@"
+ else
+ # Both dataDir and dataLogDir specified
+ "$JAVA" "-Dzookeeper.log.dir=$ZOO_LOG_DIR"
"-Dzookeeper.log.file=$ZOO_LOG_FILE" \
+ "${flags[@]}" org.apache.zookeeper.server.PurgeTxnLog "$ZOODATALOGDIR"
"$ZOODATADIR" "$@"
+ fi
else
- "$JAVA" "-Dzookeeper.log.dir=$ZOO_LOG_DIR"
"-Dzookeeper.log.file=$ZOO_LOG_FILE" "${flags[@]}" \
- org.apache.zookeeper.server.PurgeTxnLog "$ZOODATALOGDIR" "$ZOODATADIR" "$@"
+ # No config or config doesn't specify directories - pass all args to
PurgeTxnLog
+ "$JAVA" "-Dzookeeper.log.dir=$ZOO_LOG_DIR"
"-Dzookeeper.log.file=$ZOO_LOG_FILE" \
+ "${flags[@]}" org.apache.zookeeper.server.PurgeTxnLog "$@"
fi
Review Comment:
The complexity of this if/else can be entirely removed if you just build up
the command-line arguments. Combining with my earlier suggestion to add the
other system properties to the `flags` array, this simplifies to:
```suggestion
directories=()
[[ -n $ZOODATADIR ]] && directories=("$ZOODATADIR")
[[ -n $ZOODATALOGDIR ]] && directories=("$ZOODATALOGDIR" "${directories[@]}")
"$JAVA" "${flags[@]}" org.apache.zookeeper.server.PurgeTxnLog
"${directories[@]}" "$@"
```
This also covers the case where only `dataLogDir` is set, which I think
wasn't covered before.
--
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]
