Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8456 )
Change subject: IMPALA-6148: Specifying thirdparty deps as URLs ...................................................................... Patch Set 4: (6 comments) Thanks for the reviews! I think I addressed/responded all the 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: # python bootstrap_toolchain.py > Not a blocker, but would it make sense to find/replace print -> logging thr Done http://gerrit.cloudera.org:8080/#/c/8456/2/bin/bootstrap_toolchain.py@82 PS2, Line 82: if not self.version: > Nit: Probably a bit pedantic, but catching a specific exception only to rai I fixed this by getting rid of catching KeyError entirely. http://gerrit.cloudera.org:8080/#/c/8456/2/bin/bootstrap_toolchain.py@112 PS2, Line 112: if re.search(k, release): > Also, would it be appropriate to cache the value as an attribute of Package You can't trivially cache v because it depends on the argument 'release'. I'm happy to revert this function to its previous incarnation. The overall logic here is complicated enough that I don't want to do a further refactor of moving the URL-creation stuff into Package in the same commit. http://gerrit.cloudera.org:8080/#/c/8456/2/bin/bootstrap_toolchain.py@373 PS2, Line 373: > Nit: trailing white space Done http://gerrit.cloudera.org:8080/#/c/8456/2/bin/bootstrap_toolchain.py@399 PS2, Line 399: logging.basicConfig(level=logging.INFO, > Nice. Done http://gerrit.cloudera.org:8080/#/c/8456/3/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/8456/3/bin/impala-config.sh@138 PS3, Line 138: # Download URLs for toolchain dependencies can be overridden by : # IMPALA_<PACKAGE>_URLs in impala-config-*.sh. We unset them here first: : for var in "${!IMPALA_@}"; do : if [[ "$var" == IMPALA_*_URL ]]; then : unset $var : fi : done > If I understand this correctly, setting these in the environment won't work I updated commit message and some comments to reflect this. I think it's appropriate for IMPALA_HIVE_URL and IMPALA_HIVE_VERSION to have the same lifecycle, which in this case is impala-config*.sh. -- 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: 4 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: Wed, 08 Nov 2017 18:10:05 +0000 Gerrit-HasComments: Yes
