Tim Armstrong has posted comments on this change. Change subject: IMPALA-3223: Supports download of CDH components from S3. ......................................................................
Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/3333/2/bin/bootstrap_toolchain.py File bin/bootstrap_toolchain.py: Line 15: # Bootstrapping the native toolchain with prebuilt binaries This comment and the script name is getting increasingly inaccurate now that it's doing more things than just downloading prebuilt binaries. Let's update the comment. Should we rename the script to something like download_dependencies.py? Ok to ignore. Line 288: def download_cdh_components(toolchain_root, cdh_components): bootstrap_toolchain can be run every time buildall.sh is run (for clean or incremental builds), so it seems like a problem to depend on the internet, e.g. if I want to develop offline or just don't want the additional build latency of the network round-trips. I run buildall.sh pretty frequently in my workflow, for example. The native toolchain bootstrap logic only downloads missing versions of packages, so it only needs the internet for the initial bootstrap. People could toggle the value of DOWNLOAD_CDH_COMPONENTS to get it to do the desired thing, but we don't generally use environment variables that way in the build system. So I think by default we should use the download-only-if-missing behaviour. The md5sum-checking behaviour seems useful in some cases, but I think it should be opt-in. Line 342: Extra line in comment. http://gerrit.cloudera.org:8080/#/c/3333/2/bin/impala-config.sh File bin/impala-config.sh: Line 49: : ${SKIP_TOOLCHAIN_BOOTSTRAP=false} Maybe a one-liner comment? http://gerrit.cloudera.org:8080/#/c/3333/2/buildall.sh File buildall.sh: Line 253: if [ "$?" == "0" ]; then More concisely: if $IMPALA_HOME/bin/bootstrap_toolchain.py; then Line 255: else Actually, set -e already fails the script if the bootstrap_toolchain.py command returns non-zero, so the else branch is dead code here. -- To view, visit http://gerrit.cloudera.org:8080/3333 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16fa79db0005554cc0a116e74775647ba99f8dda Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
