David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/8456 )
Change subject: IMPALA-6148: Specifying thirdparty deps as URLs ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/8456/2/bin/bootstrap_toolchain.py File bin/bootstrap_toolchain.py: http://gerrit.cloudera.org:8080/#/c/8456/2/bin/bootstrap_toolchain.py@36 PS2, Line 36: import logging Not a blocker, but would it make sense to find/replace print -> logging throughout? http://gerrit.cloudera.org:8080/#/c/8456/2/bin/bootstrap_toolchain.py@82 PS2, Line 82: raise Exception("Could not find version for {0} in environment var {1}".format( Nit: Probably a bit pedantic, but catching a specific exception only to raise a generic one looks weird. Perhaps... msg = "Could not find version for {0} in environment var {1}".format(name, version_env_var) raise KeyError(msg) ...or something similar. http://gerrit.cloudera.org:8080/#/c/8456/2/bin/bootstrap_toolchain.py@373 PS2, Line 373: pkg_directory = package_directory(cdh_components_home, component.name, Nit: trailing white space http://gerrit.cloudera.org:8080/#/c/8456/2/bin/bootstrap_toolchain.py@399 PS2, Line 399: logging.getLogger("sh").setLevel(logging.WARNING) Nice. -- To view, visit http://gerrit.cloudera.org:8080/8456 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422 Gerrit-Change-Number: 8456 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-Comment-Date: Tue, 07 Nov 2017 01:59:37 +0000 Gerrit-HasComments: Yes
