impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3223: Supports download of CDH components from S3.
Date Wed, 08 Jun 2016 04:17:50 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message