[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set
Jim Apple has posted comments on this change.
Change subject: Don't assume that AUX exists just because a shell variable is
set
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4563/2/tests/run-custom-cluster-tests.sh
File tests/run-custom-cluster-tests.sh:
PS2, Line 40: then
: impala-py.test custom_cluster/ authorization/
"${AUX_CUSTOM_DIR}" \
:
--junitxml="${RESULTS_DIR}/TEST-impala-custom-cluster.xml" \
:
--resultlog="${RESULTS_DIR}/TEST-impala-custom-cluster.log" "$@"
: else
: impala-py.test custom_cluster/ authorization/ \
:
--junitxml="${RESULTS_DIR}/TEST-impala-custom-cluster.xml" \
:
--resultlog="${RESULTS_DIR}/TEST-impala-custom-cluster.log" "$@"
: fi
> I don't like the code repetition, and it's not clear right away what the di
Done
--
To view, visit http://gerrit.cloudera.org:8080/4563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: David Knupp
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Michael Brown
Gerrit-Reviewer: Taras Bobrovytsky
Gerrit-HasComments: Yes
[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set
Hello Michael Brown, Alex Behm, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4563 to look at the new patch set (#3). Change subject: Don't assume that AUX exists just because a shell variable is set .. Don't assume that AUX exists just because a shell variable is set This caused breakage of the custom cluster tests - bin/impala-config.sh sets IMPALA_AUX_TEST_HOME to $IMPALA_HOME/../Impala-auxiliary-tests, unless it is already set, but this directory is not guaranteed to exist. bin/run-custom-cluster-tests.sh was expecting that if IMPALA_AUX_TEST_HOME is set, then that directory must exist. While I'm changing bin/run-custom-cluster-tests.sh, add some quotes to protect against paths with spaces in them. Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867 --- M tests/run-custom-cluster-tests.sh 1 file changed, 16 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/4563/3 -- To view, visit http://gerrit.cloudera.org:8080/4563 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Taras Bobrovytsky
[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set
Taras Bobrovytsky has posted comments on this change.
Change subject: Don't assume that AUX exists just because a shell variable is
set
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4563/2/tests/run-custom-cluster-tests.sh
File tests/run-custom-cluster-tests.sh:
PS2, Line 40: then
: impala-py.test custom_cluster/ authorization/
"${AUX_CUSTOM_DIR}" \
:
--junitxml="${RESULTS_DIR}/TEST-impala-custom-cluster.xml" \
:
--resultlog="${RESULTS_DIR}/TEST-impala-custom-cluster.log" "$@"
: else
: impala-py.test custom_cluster/ authorization/ \
:
--junitxml="${RESULTS_DIR}/TEST-impala-custom-cluster.xml" \
:
--resultlog="${RESULTS_DIR}/TEST-impala-custom-cluster.log" "$@"
: fi
I don't like the code repetition, and it's not clear right away what the
difference is between the cases. How about something like:
ARGS=("custom_cluster/ authorization/ ");
if [ -d "${AUX_CUSTOM_DIR}" ] then
ARGS+=(${AUX_CUSTOM_DIR} )
fi
ARGS+=("--junitxml="${RESULTS_DIR}/TEST-impala-custom-cluster.xml ")
ARGS+=("--resultlog="${RESULTS_DIR}/TEST-impala-custom-cluster.log ")
ARGS+=("$@")
impala-py.test "${ARGS[@]}"
--
To view, visit http://gerrit.cloudera.org:8080/4563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: David Knupp
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Michael Brown
Gerrit-Reviewer: Taras Bobrovytsky
Gerrit-HasComments: Yes
[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set
Jim Apple has posted comments on this change. Change subject: Don't assume that AUX exists just because a shell variable is set .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/4563/1/tests/run-custom-cluster-tests.sh File tests/run-custom-cluster-tests.sh: PS1, Line 42: TEST-impala-custom- pytest sees the zero-length argument and tries to use it, eventually indexing into it at location 0, causing an IndexError. PS2 does things differently. I have testing both with and without an aux directory. -- To view, visit http://gerrit.cloudera.org:8080/4563 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-HasComments: Yes
[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set
Hello Michael Brown, Alex Behm, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4563 to look at the new patch set (#2). Change subject: Don't assume that AUX exists just because a shell variable is set .. Don't assume that AUX exists just because a shell variable is set This caused breakage of the custom cluster tests - bin/impala-config.sh sets IMPALA_AUX_TEST_HOME to $IMPALA_HOME/../Impala-auxiliary-tests, unless it is already set, but this directory is not guaranteed to exist. bin/run-custom-cluster-tests.sh was expecting that if IMPALA_AUX_TEST_HOME is set, then that directory must exist. While I'm changing bin/run-custom-cluster-tests.sh, add some quotes to protect against paths with spaces in them. Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867 --- M tests/run-custom-cluster-tests.sh 1 file changed, 17 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/4563/2 -- To view, visit http://gerrit.cloudera.org:8080/4563 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown
[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set
Alex Behm has posted comments on this change. Change subject: Don't assume that AUX exists just because a shell variable is set .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4563 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-HasComments: No
[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set
Michael Brown has posted comments on this change. Change subject: Don't assume that AUX exists just because a shell variable is set .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/4563 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-HasComments: No
[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set
Jim Apple has posted comments on this change. Change subject: Don't assume that AUX exists just because a shell variable is set .. Patch Set 1: I tested this when AUX is present; it passed. I'm now testing when AUX is absent. -- To view, visit http://gerrit.cloudera.org:8080/4563 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set
Jim Apple has posted comments on this change. Change subject: Don't assume that AUX exists just because a shell variable is set .. Patch Set 1: Testing now. I expect the tests to take a few hours. -- To view, visit http://gerrit.cloudera.org:8080/4563 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set
Jim Apple has uploaded a new change for review. http://gerrit.cloudera.org:8080/4563 Change subject: Don't assume that AUX exists just because a shell variable is set .. Don't assume that AUX exists just because a shell variable is set This caused breakage of the custom cluster tests - bin/impala-config.sh sets IMPALA_AUX_TEST_HOME to $IMPALA_HOME/../Impala-auxiliary-tests, unless it is already set, but this directory is not guaranteed to exist. bin/run-custom-cluster-tests.sh was expecting that if IMPALA_AUX_TEST_HOME is set, then that directory must exist. While I'm changing bin/run-custom-cluster-tests.sh, add some quotes to protect against paths with spaces in them. Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867 --- M tests/run-custom-cluster-tests.sh 1 file changed, 8 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/4563/1 -- To view, visit http://gerrit.cloudera.org:8080/4563 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple
