impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
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:

File bin/

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 Ok to ignore.

Line 288: def download_cdh_components(toolchain_root, cdh_components):
bootstrap_toolchain can be run every time 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
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.
File bin/

Maybe a one-liner comment?

Line 253:   if [ "$?" == "0" ]; then
More concisely:

if $IMPALA_HOME/bin/; then

Line 255:   else
Actually, set -e already fails the script if the command returns non-zero,
so the else branch is dead code here.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa79db0005554cc0a116e74775647ba99f8dda
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message