Tim Armstrong has posted comments on this change. Change subject: IMPALA-4006 impala-config.sh contains dangerous rm -rf statements ......................................................................
Patch Set 1: (13 comments) This seems like a good practice, but I'm not clear on how you decided on whether to use quotes or not, it seems like either you missed some places or I'm not understanding the criteria used. I pointed out a few places but not all where this was the case. Can you clarify and then I'll do another pass. http://gerrit.cloudera.org:8080/#/c/4078/1/bin/clean.sh File bin/clean.sh: It doesn't look like you quoted any instances of CLUSTER_DIR/IMPALA_HOME etc in this file - was that deliberate? Line 48: rm -rf "${IMPALA_LOGS_DIR}"/* Is this much safer if $IMPALA_LOGS_DIR is empty? http://gerrit.cloudera.org:8080/#/c/4078/1/bin/impala-config.sh File bin/impala-config.sh: Line 39: export IMPALA_HOME=$(dirname "$(cd $(dirname ${(%):-%x}) && pwd)") Why redirect for bash but not zsh? Line 120: if [[ -z "$KUDU_BUILD_DIR" ]]; then Why quote KUDU_BUILD_DIR here but not above? http://gerrit.cloudera.org:8080/#/c/4078/1/bin/impala-python File bin/impala-python: Line 2: source "$(dirname "$0")/impala-python-common.sh" The nested quoting here seems strange - does this work as intended? http://gerrit.cloudera.org:8080/#/c/4078/1/bin/run-all-tests.sh File bin/run-all-tests.sh: Line 173: ${IMPALA_HOME}/bin/start-impala-cluster.py --log_dir=${IMPALA_EE_TEST_LOGS_DIR} \ Were these instances deliberately omitted? http://gerrit.cloudera.org:8080/#/c/4078/1/buildall.sh File buildall.sh: Line 279: if [ -e $HADOOP_LZO/build/native/Linux-*-*/lib/libgplcompression.so ] Did you deliberately omit quoting HADOOP_LZO here? Line 281: cp $HADOOP_LZO/build/native/Linux-*-*/lib/libgplcompression.* $HADOOP_HOME/lib/native What about HADOOP_HOME? PS1, Line 312: METASTORE_SNAPSHOT_FILE Quote this path too? PS1, Line 323: IMPALA_LZO Quote IMPALA_LZO? Line 400: if [[ $SNAPSHOT_FILE && $METASTORE_SNAPSHOT_FILE ]]; then Quote the SNAPSHOT_FILE paths? PS1, Line 407: ${CREATE_LOAD_DATA_ARGS} I guess this will just expand the paths anyway so there may not be any point quoting them. Line 429: echo " . ${IMPALA_HOME}/bin/impala-config.sh" quote IMPALA_HOME? -- To view, visit http://gerrit.cloudera.org:8080/4078 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7503794180dee99eeb979e67f34e3b2edade70fe Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zoltan Ivanfi <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
