Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10068 )
Change subject: Allow bootstrap_system.sh to work on existing impala/ repo ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/10068/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10068/4//COMMIT_MSG@27 PS4, Line 27: - User has already set $IMPALA_HOME but to an invalid location: This is actually quite confusing. Is specifying a new location that does not exist considered as an invalid location? http://gerrit.cloudera.org:8080/#/c/10068/4/bin/bootstrap_system.sh File bin/bootstrap_system.sh: http://gerrit.cloudera.org:8080/#/c/10068/4/bin/bootstrap_system.sh@113 PS4, Line 113: if [[ -e $REPO_BIN_DIR/../.git ]] I don't think this works quite well. What happens when someone downloads the Impala distribution (the .tar.gz) that doesn't have .git? Also what happens when somebody runs bootstrap_system.sh not from the bin directory, e.g. ../../somedir/../bin/bootstrap_system.sh? http://gerrit.cloudera.org:8080/#/c/10068/4/bin/bootstrap_system.sh@114 PS4, Line 114: then I think the existing bootstrap_system.sh already supports the following scenarios. - IMPALA_HOME not set: use ~/Impala regardless whether there is an existing Iimpala repo cloned or downloaded somewhere. - IMPALA_HOME set: * path exists: use that location regardless whether it's valid or not. I'm not sure if we really need to take care of an invalid path. It's kinda similar to when specifying an invalid JAVA_HOME, GOROOT, etc. * path does not exist: clone it to the location specified in IMPALA_HOME When IMPALA_HOME is not set and we want to use an existing Impala (be it cloned from git or downloaded from a tarball), I think we should mandate that IMPALA_HOME needs to be set instead of trying to figure out whether we have an existing Impala or not. IMO figuring out whether an existing Impala exists can be pretty error-prone. -- To view, visit http://gerrit.cloudera.org:8080/10068 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If18a34ae54933d9ced2069602a8dca172826b18c Gerrit-Change-Number: 10068 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Wed, 02 May 2018 04:24:44 +0000 Gerrit-HasComments: Yes
