You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/08/03 13:35:32 UTC
[GitHub] [zookeeper] tanisdlj opened a new pull request #1420: Replaced weird comparison of vars
tanisdlj opened a new pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420
To improve readability.
-n $var instead of "x$var" = "x"
Also equalized a bit the indentation and format of "if" statements along the script
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ctubbsii commented on pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#issuecomment-668079355
> if we are going to do this change, we should probably do it in the rest of the scripts in the bin section all at once. (although, as @ctubbsii's review shows, it can be tricky to get right. we don't have good tests for the scripts.) TBH it's a rather common construct in bash, and the "; then" is also rather subjective; however, we have a mix of styles in the bin directory, with the "; then" being slightly more common. it would be good to have a common format for our shell scripts.
You also have to be careful if you want to support POSIX-only, or bash, and if bash, then bash 3 only (because macOS) or 4+ (because everybody else).
One tool I've found *very* useful is [ShellCheck](https://github.com/koalaman/shellcheck). It's available in most Linux distros, and also an online version at https://www.shellcheck.net/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ctubbsii commented on pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#issuecomment-671390370
> Changed the branch name to follow the guidelines. Follow up here please :).
> #1426
Why would the source branch name matter? That's in the contributor space, not in the receiving project's space.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] breed commented on pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
breed commented on pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#issuecomment-668073727
if we are going to do this change, we should probably do it in the rest of the scripts in the bin section all at once. (although, as @ctubbsii's review shows, it can be tricky to get right. we don't have good tests for the scripts.) TBH it's a rather common construct in bash, and the "; then" is also rather subjective; however, we have a mix of styles in the bin directory, with the "; then" being slightly more common. it would be good to have a common format for our shell scripts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tanisdlj commented on a change in pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
tanisdlj commented on a change in pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#discussion_r464563193
##########
File path: bin/zkServer.sh
##########
@@ -37,73 +37,62 @@ fi
# up the JVM to accept JMX remote management:
# http://java.sun.com/javase/6/docs/technotes/guides/management/agent.html
# by default we allow local JMX connections
-if [ "x$JMXLOCALONLY" = "x" ]
-then
- JMXLOCALONLY=false
+if [ -n "$JMXLOCALONLY" ]; then
+ JMXLOCALONLY=false
Review comment:
Yeah, it got me pretty messed up the x=x xDD. You are absolutely right, I'm gonna change it 👍
Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
nkalmar commented on pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#issuecomment-670040936
Good fix, great points in the review. Thanks guys!
But please create a jira ticket @tanisdlj , this is by all means big enough change to have one.
By the way, do you plan to take a look at the other bash scripts 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#discussion_r464572369
##########
File path: bin/zkServer.sh
##########
@@ -37,73 +37,62 @@ fi
# up the JVM to accept JMX remote management:
# http://java.sun.com/javase/6/docs/technotes/guides/management/agent.html
# by default we allow local JMX connections
-if [ "x$JMXLOCALONLY" = "x" ]
-then
- JMXLOCALONLY=false
+if [ -z "$JMXLOCALONLY" ]; then
+ JMXLOCALONLY=false
fi
-if [ "x$JMXDISABLE" = "x" ] || [ "$JMXDISABLE" = 'false' ]
-then
+if [ -z "$JMXDISABLE" ] || [ "$JMXDISABLE" == 'false' ]; then
echo "ZooKeeper JMX enabled by default" >&2
- if [ "x$JMXPORT" = "x" ]
- then
+ if [ -z "$JMXPORT" ]; then
# for some reason these two options are necessary on jdk6 on Ubuntu
# accord to the docs they are not necessary, but otw jconsole cannot
# do a local attach
ZOOMAIN="-Dcom.sun.management.jmxremote -Dcom.sun.management.jmxremote.local.only=$JMXLOCALONLY org.apache.zookeeper.server.quorum.QuorumPeerMain"
else
- if [ "x$JMXAUTH" = "x" ]
- then
+ if [ -z "$JMXAUTH" ]; then
JMXAUTH=false
fi
- if [ "x$JMXSSL" = "x" ]
- then
+ if [ -z "$JMXSSL" ]; then
JMXSSL=false
fi
- if [ "x$JMXLOG4J" = "x" ]
- then
+ if [ -z "$JMXLOG4J" ]; then
JMXLOG4J=true
fi
echo "ZooKeeper remote JMX Port set to $JMXPORT" >&2
echo "ZooKeeper remote JMX authenticate set to $JMXAUTH" >&2
echo "ZooKeeper remote JMX ssl set to $JMXSSL" >&2
echo "ZooKeeper remote JMX log4j set to $JMXLOG4J" >&2
- if [ "x$JMXHOSTNAME" = "x" ]
- then
+ if [ -z "$JMXHOSTNAME" ]; then
ZOOMAIN="-Dcom.sun.management.jmxremote -Dcom.sun.management.jmxremote.port=$JMXPORT -Dcom.sun.management.jmxremote.authenticate=$JMXAUTH -Dcom.sun.management.jmxremote.ssl=$JMXSSL -Dzookeeper.jmx.log4j.disable=$JMXLOG4J org.apache.zookeeper.server.quorum.QuorumPeerMain"
else
echo "ZooKeeper remote JMX Hostname set to $JMXHOSTNAME" >&2
ZOOMAIN="-Dcom.sun.management.jmxremote -Djava.rmi.server.hostname=$JMXHOSTNAME -Dcom.sun.management.jmxremote.port=$JMXPORT -Dcom.sun.management.jmxremote.authenticate=$JMXAUTH -Dcom.sun.management.jmxremote.ssl=$JMXSSL -Dzookeeper.jmx.log4j.disable=$JMXLOG4J org.apache.zookeeper.server.quorum.QuorumPeerMain"
fi
fi
else
- echo "JMX disabled by user request" >&2
- ZOOMAIN="org.apache.zookeeper.server.quorum.QuorumPeerMain"
+ echo "JMX disabled by user request" >&2
+ ZOOMAIN="org.apache.zookeeper.server.quorum.QuorumPeerMain"
fi
-if [ "x$SERVER_JVMFLAGS" != "x" ]
-then
- JVMFLAGS="$SERVER_JVMFLAGS $JVMFLAGS"
+if [ -z "$SERVER_JVMFLAGS" ]; then
+ JVMFLAGS="$SERVER_JVMFLAGS $JVMFLAGS"
fi
-if [ "x$2" != "x" ]
-then
- ZOOCFG="$ZOOCFGDIR/$2"
+if [ -z "$2" ]; then
Review comment:
This one should be `-n`, since it was a `!=`.
```suggestion
if [ -n "$2" ]; then
```
##########
File path: bin/zkServer.sh
##########
@@ -37,73 +37,62 @@ fi
# up the JVM to accept JMX remote management:
# http://java.sun.com/javase/6/docs/technotes/guides/management/agent.html
# by default we allow local JMX connections
-if [ "x$JMXLOCALONLY" = "x" ]
-then
- JMXLOCALONLY=false
+if [ -z "$JMXLOCALONLY" ]; then
+ JMXLOCALONLY=false
fi
-if [ "x$JMXDISABLE" = "x" ] || [ "$JMXDISABLE" = 'false' ]
-then
+if [ -z "$JMXDISABLE" ] || [ "$JMXDISABLE" == 'false' ]; then
echo "ZooKeeper JMX enabled by default" >&2
- if [ "x$JMXPORT" = "x" ]
- then
+ if [ -z "$JMXPORT" ]; then
# for some reason these two options are necessary on jdk6 on Ubuntu
# accord to the docs they are not necessary, but otw jconsole cannot
# do a local attach
ZOOMAIN="-Dcom.sun.management.jmxremote -Dcom.sun.management.jmxremote.local.only=$JMXLOCALONLY org.apache.zookeeper.server.quorum.QuorumPeerMain"
else
- if [ "x$JMXAUTH" = "x" ]
- then
+ if [ -z "$JMXAUTH" ]; then
JMXAUTH=false
fi
- if [ "x$JMXSSL" = "x" ]
- then
+ if [ -z "$JMXSSL" ]; then
JMXSSL=false
fi
- if [ "x$JMXLOG4J" = "x" ]
- then
+ if [ -z "$JMXLOG4J" ]; then
JMXLOG4J=true
fi
echo "ZooKeeper remote JMX Port set to $JMXPORT" >&2
echo "ZooKeeper remote JMX authenticate set to $JMXAUTH" >&2
echo "ZooKeeper remote JMX ssl set to $JMXSSL" >&2
echo "ZooKeeper remote JMX log4j set to $JMXLOG4J" >&2
- if [ "x$JMXHOSTNAME" = "x" ]
- then
+ if [ -z "$JMXHOSTNAME" ]; then
ZOOMAIN="-Dcom.sun.management.jmxremote -Dcom.sun.management.jmxremote.port=$JMXPORT -Dcom.sun.management.jmxremote.authenticate=$JMXAUTH -Dcom.sun.management.jmxremote.ssl=$JMXSSL -Dzookeeper.jmx.log4j.disable=$JMXLOG4J org.apache.zookeeper.server.quorum.QuorumPeerMain"
else
echo "ZooKeeper remote JMX Hostname set to $JMXHOSTNAME" >&2
ZOOMAIN="-Dcom.sun.management.jmxremote -Djava.rmi.server.hostname=$JMXHOSTNAME -Dcom.sun.management.jmxremote.port=$JMXPORT -Dcom.sun.management.jmxremote.authenticate=$JMXAUTH -Dcom.sun.management.jmxremote.ssl=$JMXSSL -Dzookeeper.jmx.log4j.disable=$JMXLOG4J org.apache.zookeeper.server.quorum.QuorumPeerMain"
fi
fi
else
- echo "JMX disabled by user request" >&2
- ZOOMAIN="org.apache.zookeeper.server.quorum.QuorumPeerMain"
+ echo "JMX disabled by user request" >&2
+ ZOOMAIN="org.apache.zookeeper.server.quorum.QuorumPeerMain"
fi
-if [ "x$SERVER_JVMFLAGS" != "x" ]
-then
- JVMFLAGS="$SERVER_JVMFLAGS $JVMFLAGS"
+if [ -z "$SERVER_JVMFLAGS" ]; then
Review comment:
This one should be `-n`, since it was a `!=`.
```suggestion
if [ -n "$SERVER_JVMFLAGS" ]; then
```
##########
File path: bin/zkServer.sh
##########
@@ -122,51 +111,51 @@ ZOO_DATALOGDIR="$($GREP "^[[:space:]]*dataLogDir" "$ZOOCFG" | sed -e 's/.*=//')"
# iff autocreate is turned off and the datadirs don't exist fail
# immediately as we can't create the PID file, etc..., anyway.
-if [ -n "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
- if [ ! -d "$ZOO_DATADIR/version-2" ]; then
- echo "ZooKeeper data directory is missing at $ZOO_DATADIR fix the path or run initialize"
- exit 1
- fi
+if [ -z "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
Review comment:
I'm not sure this should have been changed.
```suggestion
if [ -n "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
```
##########
File path: bin/zkServer.sh
##########
@@ -236,43 +223,41 @@ restart)
status)
# -q is necessary on some versions of linux where nc returns too quickly, and no stat result is output
isSSL="false"
- clientPortAddress=`$GREP "^[[:space:]]*clientPortAddress[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//'`
- if ! [ $clientPortAddress ]
- then
- clientPortAddress="localhost"
+ clientPortAddress=$($GREP "^[[:space:]]*clientPortAddress[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//')
+ if ! [ "$clientPortAddress" ]; then
Review comment:
I think this is trying to do:
```suggestion
if [ -z "$clientPortAddress" ]; then
```
I'm not exactly, sure, though. This is used in a few places.
##########
File path: bin/zkServer.sh
##########
@@ -122,51 +111,51 @@ ZOO_DATALOGDIR="$($GREP "^[[:space:]]*dataLogDir" "$ZOOCFG" | sed -e 's/.*=//')"
# iff autocreate is turned off and the datadirs don't exist fail
# immediately as we can't create the PID file, etc..., anyway.
-if [ -n "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
- if [ ! -d "$ZOO_DATADIR/version-2" ]; then
- echo "ZooKeeper data directory is missing at $ZOO_DATADIR fix the path or run initialize"
- exit 1
- fi
+if [ -z "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
+ if [ ! -d "$ZOO_DATADIR/version-2" ]; then
+ echo "ZooKeeper data directory is missing at $ZOO_DATADIR fix the path or run initialize"
+ exit 1
+ fi
- if [ -n "$ZOO_DATALOGDIR" ] && [ ! -d "$ZOO_DATALOGDIR/version-2" ]; then
- echo "ZooKeeper txnlog directory is missing at $ZOO_DATALOGDIR fix the path or run initialize"
- exit 1
- fi
- ZOO_DATADIR_AUTOCREATE="-Dzookeeper.datadir.autocreate=false"
+ if [ -z "$ZOO_DATALOGDIR" ] && [ ! -d "$ZOO_DATALOGDIR/version-2" ]; then
+ echo "ZooKeeper txnlog directory is missing at $ZOO_DATALOGDIR fix the path or run initialize"
+ exit 1
+ fi
+ ZOO_DATADIR_AUTOCREATE="-Dzookeeper.datadir.autocreate=false"
fi
if [ -z "$ZOOPIDFILE" ]; then
- if [ ! -d "$ZOO_DATADIR" ]; then
- mkdir -p "$ZOO_DATADIR"
- fi
- ZOOPIDFILE="$ZOO_DATADIR/zookeeper_server.pid"
+ if [ ! -d "$ZOO_DATADIR" ]; then
+ mkdir -p "$ZOO_DATADIR"
+ fi
+ ZOOPIDFILE="$ZOO_DATADIR/zookeeper_server.pid"
else
- # ensure it exists, otw stop will fail
- mkdir -p "$(dirname "$ZOOPIDFILE")"
+ # ensure it exists, otw stop will fail
+ mkdir -p "$(dirname "$ZOOPIDFILE")"
fi
if [ ! -w "$ZOO_LOG_DIR" ] ; then
-mkdir -p "$ZOO_LOG_DIR"
+ mkdir -p "$ZOO_LOG_DIR"
fi
ZOO_LOG_FILE=zookeeper-$USER-server-$HOSTNAME.log
_ZOO_DAEMON_OUT="$ZOO_LOG_DIR/zookeeper-$USER-server-$HOSTNAME.out"
case $1 in
start)
- echo -n "Starting zookeeper ... "
+ echo -n "Starting zookeeper ... "
if [ -f "$ZOOPIDFILE" ]; then
- if kill -0 `cat "$ZOOPIDFILE"` > /dev/null 2>&1; then
- echo $command already running as process `cat "$ZOOPIDFILE"`.
- exit 1
+ if kill -0 $(cat "$ZOOPIDFILE") > /dev/null 2>&1; then
+ echo "$command" already running as process $(cat "$ZOOPIDFILE").
+ exit 1
fi
fi
nohup "$JAVA" $ZOO_DATADIR_AUTOCREATE "-Dzookeeper.log.dir=${ZOO_LOG_DIR}" \
"-Dzookeeper.log.file=${ZOO_LOG_FILE}" "-Dzookeeper.root.logger=${ZOO_LOG4J_PROP}" \
-XX:+HeapDumpOnOutOfMemoryError -XX:OnOutOfMemoryError='kill -9 %p' \
- -cp "$CLASSPATH" $JVMFLAGS $ZOOMAIN "$ZOOCFG" > "$_ZOO_DAEMON_OUT" 2>&1 < /dev/null &
- if [ $? -eq 0 ]
- then
+ -cp "$CLASSPATH" "$JVMFLAGS" $ZOOMAIN "$ZOOCFG" > "$_ZOO_DAEMON_OUT" 2>&1 < /dev/null &
Review comment:
Even though this one will trigger a ShellCheck warning, it should *not* be quoted. It is intentionally split, since the variable is not an array and it's expected that multiple JVM flags can be provided in a single variable. This is normal for this kind of variable. You can probably configure ShellCheck to ignore this one, but it will probably be too aggressive and ignore the entire line. It's probably best to just add a comment that `$JVMFLAGS` should not be double-quoted.
```suggestion
-cp "$CLASSPATH" $JVMFLAGS $ZOOMAIN "$ZOOCFG" > "$_ZOO_DAEMON_OUT" 2>&1 < /dev/null &
```
Also, `-cp` shouldn't be used at all. Instead, the `CLASSPATH` environment variable should be exported. The `-cp` method for passing the class path makes the command-line insanely long, sometimes exceeding the system's maximum command-line length. However, that is a separate issue and should be fixed separately.
##########
File path: bin/zkServer.sh
##########
@@ -122,51 +111,51 @@ ZOO_DATALOGDIR="$($GREP "^[[:space:]]*dataLogDir" "$ZOOCFG" | sed -e 's/.*=//')"
# iff autocreate is turned off and the datadirs don't exist fail
# immediately as we can't create the PID file, etc..., anyway.
-if [ -n "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
- if [ ! -d "$ZOO_DATADIR/version-2" ]; then
- echo "ZooKeeper data directory is missing at $ZOO_DATADIR fix the path or run initialize"
- exit 1
- fi
+if [ -z "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
+ if [ ! -d "$ZOO_DATADIR/version-2" ]; then
+ echo "ZooKeeper data directory is missing at $ZOO_DATADIR fix the path or run initialize"
+ exit 1
+ fi
- if [ -n "$ZOO_DATALOGDIR" ] && [ ! -d "$ZOO_DATALOGDIR/version-2" ]; then
- echo "ZooKeeper txnlog directory is missing at $ZOO_DATALOGDIR fix the path or run initialize"
- exit 1
- fi
- ZOO_DATADIR_AUTOCREATE="-Dzookeeper.datadir.autocreate=false"
+ if [ -z "$ZOO_DATALOGDIR" ] && [ ! -d "$ZOO_DATALOGDIR/version-2" ]; then
+ echo "ZooKeeper txnlog directory is missing at $ZOO_DATALOGDIR fix the path or run initialize"
+ exit 1
+ fi
+ ZOO_DATADIR_AUTOCREATE="-Dzookeeper.datadir.autocreate=false"
fi
if [ -z "$ZOOPIDFILE" ]; then
- if [ ! -d "$ZOO_DATADIR" ]; then
- mkdir -p "$ZOO_DATADIR"
- fi
- ZOOPIDFILE="$ZOO_DATADIR/zookeeper_server.pid"
+ if [ ! -d "$ZOO_DATADIR" ]; then
+ mkdir -p "$ZOO_DATADIR"
+ fi
+ ZOOPIDFILE="$ZOO_DATADIR/zookeeper_server.pid"
else
- # ensure it exists, otw stop will fail
- mkdir -p "$(dirname "$ZOOPIDFILE")"
+ # ensure it exists, otw stop will fail
+ mkdir -p "$(dirname "$ZOOPIDFILE")"
fi
if [ ! -w "$ZOO_LOG_DIR" ] ; then
-mkdir -p "$ZOO_LOG_DIR"
+ mkdir -p "$ZOO_LOG_DIR"
fi
ZOO_LOG_FILE=zookeeper-$USER-server-$HOSTNAME.log
_ZOO_DAEMON_OUT="$ZOO_LOG_DIR/zookeeper-$USER-server-$HOSTNAME.out"
case $1 in
start)
- echo -n "Starting zookeeper ... "
+ echo -n "Starting zookeeper ... "
if [ -f "$ZOOPIDFILE" ]; then
- if kill -0 `cat "$ZOOPIDFILE"` > /dev/null 2>&1; then
- echo $command already running as process `cat "$ZOOPIDFILE"`.
- exit 1
+ if kill -0 $(cat "$ZOOPIDFILE") > /dev/null 2>&1; then
+ echo "$command" already running as process $(cat "$ZOOPIDFILE").
Review comment:
It'd be better to quote the entire arg as a single arg, rather than pass multiple args.
```suggestion
echo "$command already running as process $(cat "$ZOOPIDFILE")."
```
##########
File path: bin/zkServer.sh
##########
@@ -122,51 +111,51 @@ ZOO_DATALOGDIR="$($GREP "^[[:space:]]*dataLogDir" "$ZOOCFG" | sed -e 's/.*=//')"
# iff autocreate is turned off and the datadirs don't exist fail
# immediately as we can't create the PID file, etc..., anyway.
-if [ -n "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
- if [ ! -d "$ZOO_DATADIR/version-2" ]; then
- echo "ZooKeeper data directory is missing at $ZOO_DATADIR fix the path or run initialize"
- exit 1
- fi
+if [ -z "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
+ if [ ! -d "$ZOO_DATADIR/version-2" ]; then
+ echo "ZooKeeper data directory is missing at $ZOO_DATADIR fix the path or run initialize"
+ exit 1
+ fi
- if [ -n "$ZOO_DATALOGDIR" ] && [ ! -d "$ZOO_DATALOGDIR/version-2" ]; then
- echo "ZooKeeper txnlog directory is missing at $ZOO_DATALOGDIR fix the path or run initialize"
- exit 1
- fi
- ZOO_DATADIR_AUTOCREATE="-Dzookeeper.datadir.autocreate=false"
+ if [ -z "$ZOO_DATALOGDIR" ] && [ ! -d "$ZOO_DATALOGDIR/version-2" ]; then
Review comment:
This one shouldn't have changed:
```suggestion
if [ -n "$ZOO_DATALOGDIR" ] && [ ! -d "$ZOO_DATALOGDIR/version-2" ]; then
```
##########
File path: bin/zkServer.sh
##########
@@ -236,43 +223,41 @@ restart)
status)
# -q is necessary on some versions of linux where nc returns too quickly, and no stat result is output
isSSL="false"
- clientPortAddress=`$GREP "^[[:space:]]*clientPortAddress[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//'`
- if ! [ $clientPortAddress ]
- then
- clientPortAddress="localhost"
+ clientPortAddress=$($GREP "^[[:space:]]*clientPortAddress[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//')
+ if ! [ "$clientPortAddress" ]; then
+ clientPortAddress="localhost"
fi
- clientPort=`$GREP "^[[:space:]]*clientPort[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//'`
- if ! [[ "$clientPort" =~ ^[0-9]+$ ]]
- then
- dataDir=`$GREP "^[[:space:]]*dataDir" "$ZOOCFG" | sed -e 's/.*=//'`
- myid=`cat "$dataDir/myid" 2> /dev/null`
+ clientPort=$($GREP "^[[:space:]]*clientPort[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//')
+ if ! [[ "$clientPort" =~ ^[0-9]+$ ]]; then
Review comment:
This is the first time I've seen the use of the double-square brackets in this code review for a test block. It probably doesn't matter, but FWIW, if you're writing bash, it's best to use the double-square everywhere rather than the single-square braces, because then there's no ambiguity between the bash language syntax and the literal command `/usr/bin/[`. Many people don't realize that `/usr/bin/[` is actually a separate process, although I believe bash just uses its own built-in anyway, so it doesn't have the loss of performance for executing a separate process, but I'm not sure about that. In general, I prefer double over single, if you are using bash. However, I'm not sure it's worth changing this throughout the script.
##########
File path: bin/zkServer.sh
##########
@@ -202,7 +190,7 @@ start-foreground)
"${ZOO_CMD[@]}" $ZOO_DATADIR_AUTOCREATE "-Dzookeeper.log.dir=${ZOO_LOG_DIR}" \
"-Dzookeeper.log.file=${ZOO_LOG_FILE}" "-Dzookeeper.root.logger=${ZOO_LOG4J_PROP}" \
-XX:+HeapDumpOnOutOfMemoryError -XX:OnOutOfMemoryError='kill -9 %p' \
- -cp "$CLASSPATH" $JVMFLAGS $ZOOMAIN "$ZOOCFG"
+ -cp "$CLASSPATH" "$JVMFLAGS" $ZOOMAIN "$ZOOCFG"
Review comment:
Another intentionally unquoted `$JVMFLAGS`
```suggestion
-cp "$CLASSPATH" $JVMFLAGS $ZOOMAIN "$ZOOCFG"
```
##########
File path: bin/zkServer.sh
##########
@@ -282,12 +267,11 @@ status)
fi
fi
echo "Client port found: $clientPort. Client address: $clientPortAddress. Client SSL: $isSSL."
- STAT=`"$JAVA" "-Dzookeeper.log.dir=${ZOO_LOG_DIR}" "-Dzookeeper.root.logger=${ZOO_LOG4J_PROP}" "-Dzookeeper.log.file=${ZOO_LOG_FILE}" \
- -cp "$CLASSPATH" $CLIENT_JVMFLAGS $JVMFLAGS org.apache.zookeeper.client.FourLetterWordMain \
- $clientPortAddress $clientPort srvr $isSSL 2> /dev/null \
- | $GREP Mode`
- if [ "x$STAT" = "x" ]
- then
+ STAT=$("$JAVA" "-Dzookeeper.log.dir=${ZOO_LOG_DIR}" "-Dzookeeper.root.logger=${ZOO_LOG4J_PROP}" "-Dzookeeper.log.file=${ZOO_LOG_FILE}" \
+ -cp "$CLASSPATH" "$CLIENT_JVMFLAGS" "$JVMFLAGS" org.apache.zookeeper.client.FourLetterWordMain \
Review comment:
Another case of intentionally unquoted flags:
```suggestion
-cp "$CLASSPATH" $CLIENT_JVMFLAGS $JVMFLAGS org.apache.zookeeper.client.FourLetterWordMain \
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tanisdlj commented on pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
tanisdlj commented on pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#issuecomment-668167362
> > if we are going to do this change, we should probably do it in the rest of the scripts in the bin section all at once. (although, as @ctubbsii's review shows, it can be tricky to get right. we don't have good tests for the scripts.) TBH it's a rather common construct in bash, and the "; then" is also rather subjective; however, we have a mix of styles in the bin directory, with the "; then" being slightly more common. it would be good to have a common format for our shell scripts.
>
> You also have to be careful if you want to support POSIX-only, or bash, and if bash, then bash 3 only (because macOS) or 4+ (because everybody else).
>
> One tool I've found _very_ useful is [ShellCheck](https://github.com/koalaman/shellcheck). It's available in most Linux distros, and also an online version at https://www.shellcheck.net/
yup, using shellcheck myself. We have it in github also, so it's "easy" to add.
A different matter is if the script does what it's intended to.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tanisdlj commented on pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
tanisdlj commented on pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#issuecomment-670623410
Hi guys! I am in the middle of moving from one flat to another, that's why I am not super responsive, but I will try to go through your comments and fixes and apply them! @nkalmar thanks! Yeah, I hope on Sunday will create the jira issue and review this changes and, in a near future, go through the other bash scripts :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tanisdlj commented on pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
tanisdlj commented on pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#issuecomment-671368398
Changed the branch name to follow the guidelines. Follow up here please :).
https://github.com/apache/zookeeper/pull/1426
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#discussion_r467905772
##########
File path: bin/zkServer.sh
##########
@@ -236,43 +223,41 @@ restart)
status)
# -q is necessary on some versions of linux where nc returns too quickly, and no stat result is output
isSSL="false"
- clientPortAddress=`$GREP "^[[:space:]]*clientPortAddress[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//'`
- if ! [ $clientPortAddress ]
- then
- clientPortAddress="localhost"
+ clientPortAddress=$($GREP "^[[:space:]]*clientPortAddress[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//')
+ if ! [ "$clientPortAddress" ]; then
+ clientPortAddress="localhost"
fi
- clientPort=`$GREP "^[[:space:]]*clientPort[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//'`
- if ! [[ "$clientPort" =~ ^[0-9]+$ ]]
- then
- dataDir=`$GREP "^[[:space:]]*dataDir" "$ZOOCFG" | sed -e 's/.*=//'`
- myid=`cat "$dataDir/myid" 2> /dev/null`
+ clientPort=$($GREP "^[[:space:]]*clientPort[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//')
+ if ! [[ "$clientPort" =~ ^[0-9]+$ ]]; then
Review comment:
Right. My comment was inspired by the change I noticed here, but does not pertain to this change. Rather, it pertains to all the other occurrences in the script where single square brackets are used. It's up to you whether you want to migrate everything to double-square brackets in this PR or a subsequent or not at all. :smiley_cat:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tanisdlj commented on pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
tanisdlj commented on pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#issuecomment-671404872
Just to make it easier, not a big deal imho
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tanisdlj closed pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
tanisdlj closed pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tanisdlj commented on a change in pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
tanisdlj commented on a change in pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#discussion_r467900527
##########
File path: bin/zkServer.sh
##########
@@ -236,43 +223,41 @@ restart)
status)
# -q is necessary on some versions of linux where nc returns too quickly, and no stat result is output
isSSL="false"
- clientPortAddress=`$GREP "^[[:space:]]*clientPortAddress[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//'`
- if ! [ $clientPortAddress ]
- then
- clientPortAddress="localhost"
+ clientPortAddress=$($GREP "^[[:space:]]*clientPortAddress[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//')
+ if ! [ "$clientPortAddress" ]; then
+ clientPortAddress="localhost"
fi
- clientPort=`$GREP "^[[:space:]]*clientPort[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//'`
- if ! [[ "$clientPort" =~ ^[0-9]+$ ]]
- then
- dataDir=`$GREP "^[[:space:]]*dataDir" "$ZOOCFG" | sed -e 's/.*=//'`
- myid=`cat "$dataDir/myid" 2> /dev/null`
+ clientPort=$($GREP "^[[:space:]]*clientPort[^[:alpha:]]" "$ZOOCFG" | sed -e 's/.*=//')
+ if ! [[ "$clientPort" =~ ^[0-9]+$ ]]; then
Review comment:
I prefer double square brackets vs single one, but here the only change is replacing the old `Command` by $(Command). The command itself has not been changed at all
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tanisdlj commented on a change in pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
tanisdlj commented on a change in pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#discussion_r467898802
##########
File path: bin/zkServer.sh
##########
@@ -122,51 +111,51 @@ ZOO_DATALOGDIR="$($GREP "^[[:space:]]*dataLogDir" "$ZOOCFG" | sed -e 's/.*=//')"
# iff autocreate is turned off and the datadirs don't exist fail
# immediately as we can't create the PID file, etc..., anyway.
-if [ -n "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
- if [ ! -d "$ZOO_DATADIR/version-2" ]; then
- echo "ZooKeeper data directory is missing at $ZOO_DATADIR fix the path or run initialize"
- exit 1
- fi
+if [ -z "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
+ if [ ! -d "$ZOO_DATADIR/version-2" ]; then
+ echo "ZooKeeper data directory is missing at $ZOO_DATADIR fix the path or run initialize"
+ exit 1
+ fi
- if [ -n "$ZOO_DATALOGDIR" ] && [ ! -d "$ZOO_DATALOGDIR/version-2" ]; then
- echo "ZooKeeper txnlog directory is missing at $ZOO_DATALOGDIR fix the path or run initialize"
- exit 1
- fi
- ZOO_DATADIR_AUTOCREATE="-Dzookeeper.datadir.autocreate=false"
+ if [ -z "$ZOO_DATALOGDIR" ] && [ ! -d "$ZOO_DATALOGDIR/version-2" ]; then
+ echo "ZooKeeper txnlog directory is missing at $ZOO_DATALOGDIR fix the path or run initialize"
+ exit 1
+ fi
+ ZOO_DATADIR_AUTOCREATE="-Dzookeeper.datadir.autocreate=false"
fi
if [ -z "$ZOOPIDFILE" ]; then
- if [ ! -d "$ZOO_DATADIR" ]; then
- mkdir -p "$ZOO_DATADIR"
- fi
- ZOOPIDFILE="$ZOO_DATADIR/zookeeper_server.pid"
+ if [ ! -d "$ZOO_DATADIR" ]; then
+ mkdir -p "$ZOO_DATADIR"
+ fi
+ ZOOPIDFILE="$ZOO_DATADIR/zookeeper_server.pid"
else
- # ensure it exists, otw stop will fail
- mkdir -p "$(dirname "$ZOOPIDFILE")"
+ # ensure it exists, otw stop will fail
+ mkdir -p "$(dirname "$ZOOPIDFILE")"
fi
if [ ! -w "$ZOO_LOG_DIR" ] ; then
-mkdir -p "$ZOO_LOG_DIR"
+ mkdir -p "$ZOO_LOG_DIR"
fi
ZOO_LOG_FILE=zookeeper-$USER-server-$HOSTNAME.log
_ZOO_DAEMON_OUT="$ZOO_LOG_DIR/zookeeper-$USER-server-$HOSTNAME.out"
case $1 in
start)
- echo -n "Starting zookeeper ... "
+ echo -n "Starting zookeeper ... "
if [ -f "$ZOOPIDFILE" ]; then
- if kill -0 `cat "$ZOOPIDFILE"` > /dev/null 2>&1; then
- echo $command already running as process `cat "$ZOOPIDFILE"`.
- exit 1
+ if kill -0 $(cat "$ZOOPIDFILE") > /dev/null 2>&1; then
+ echo "$command" already running as process $(cat "$ZOOPIDFILE").
+ exit 1
fi
fi
nohup "$JAVA" $ZOO_DATADIR_AUTOCREATE "-Dzookeeper.log.dir=${ZOO_LOG_DIR}" \
"-Dzookeeper.log.file=${ZOO_LOG_FILE}" "-Dzookeeper.root.logger=${ZOO_LOG4J_PROP}" \
-XX:+HeapDumpOnOutOfMemoryError -XX:OnOutOfMemoryError='kill -9 %p' \
- -cp "$CLASSPATH" $JVMFLAGS $ZOOMAIN "$ZOOCFG" > "$_ZOO_DAEMON_OUT" 2>&1 < /dev/null &
- if [ $? -eq 0 ]
- then
+ -cp "$CLASSPATH" "$JVMFLAGS" $ZOOMAIN "$ZOOCFG" > "$_ZOO_DAEMON_OUT" 2>&1 < /dev/null &
Review comment:
Agree in the second part, and you are right :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#discussion_r464442751
##########
File path: bin/zkServer.sh
##########
@@ -37,73 +37,62 @@ fi
# up the JVM to accept JMX remote management:
# http://java.sun.com/javase/6/docs/technotes/guides/management/agent.html
# by default we allow local JMX connections
-if [ "x$JMXLOCALONLY" = "x" ]
-then
- JMXLOCALONLY=false
+if [ -n "$JMXLOCALONLY" ]; then
+ JMXLOCALONLY=false
Review comment:
Shouldn't this be:
```suggestion
if [ -z "$JMXLOCALONLY" ]; then
JMXLOCALONLY=false
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tanisdlj commented on a change in pull request #1420: Replaced weird comparison of vars
Posted by GitBox <gi...@apache.org>.
tanisdlj commented on a change in pull request #1420:
URL: https://github.com/apache/zookeeper/pull/1420#discussion_r467897544
##########
File path: bin/zkServer.sh
##########
@@ -122,51 +111,51 @@ ZOO_DATALOGDIR="$($GREP "^[[:space:]]*dataLogDir" "$ZOOCFG" | sed -e 's/.*=//')"
# iff autocreate is turned off and the datadirs don't exist fail
# immediately as we can't create the PID file, etc..., anyway.
-if [ -n "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
- if [ ! -d "$ZOO_DATADIR/version-2" ]; then
- echo "ZooKeeper data directory is missing at $ZOO_DATADIR fix the path or run initialize"
- exit 1
- fi
+if [ -z "$ZOO_DATADIR_AUTOCREATE_DISABLE" ]; then
+ if [ ! -d "$ZOO_DATADIR/version-2" ]; then
+ echo "ZooKeeper data directory is missing at $ZOO_DATADIR fix the path or run initialize"
+ exit 1
+ fi
- if [ -n "$ZOO_DATALOGDIR" ] && [ ! -d "$ZOO_DATALOGDIR/version-2" ]; then
- echo "ZooKeeper txnlog directory is missing at $ZOO_DATALOGDIR fix the path or run initialize"
- exit 1
- fi
- ZOO_DATADIR_AUTOCREATE="-Dzookeeper.datadir.autocreate=false"
+ if [ -z "$ZOO_DATALOGDIR" ] && [ ! -d "$ZOO_DATALOGDIR/version-2" ]; then
+ echo "ZooKeeper txnlog directory is missing at $ZOO_DATALOGDIR fix the path or run initialize"
+ exit 1
+ fi
+ ZOO_DATADIR_AUTOCREATE="-Dzookeeper.datadir.autocreate=false"
fi
if [ -z "$ZOOPIDFILE" ]; then
- if [ ! -d "$ZOO_DATADIR" ]; then
- mkdir -p "$ZOO_DATADIR"
- fi
- ZOOPIDFILE="$ZOO_DATADIR/zookeeper_server.pid"
+ if [ ! -d "$ZOO_DATADIR" ]; then
+ mkdir -p "$ZOO_DATADIR"
+ fi
+ ZOOPIDFILE="$ZOO_DATADIR/zookeeper_server.pid"
else
- # ensure it exists, otw stop will fail
- mkdir -p "$(dirname "$ZOOPIDFILE")"
+ # ensure it exists, otw stop will fail
+ mkdir -p "$(dirname "$ZOOPIDFILE")"
fi
if [ ! -w "$ZOO_LOG_DIR" ] ; then
-mkdir -p "$ZOO_LOG_DIR"
+ mkdir -p "$ZOO_LOG_DIR"
fi
ZOO_LOG_FILE=zookeeper-$USER-server-$HOSTNAME.log
_ZOO_DAEMON_OUT="$ZOO_LOG_DIR/zookeeper-$USER-server-$HOSTNAME.out"
case $1 in
start)
- echo -n "Starting zookeeper ... "
+ echo -n "Starting zookeeper ... "
if [ -f "$ZOOPIDFILE" ]; then
- if kill -0 `cat "$ZOOPIDFILE"` > /dev/null 2>&1; then
- echo $command already running as process `cat "$ZOOPIDFILE"`.
- exit 1
+ if kill -0 $(cat "$ZOOPIDFILE") > /dev/null 2>&1; then
+ echo "$command" already running as process $(cat "$ZOOPIDFILE").
Review comment:
Agree
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org