[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
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 8: > Patch Set 6: > > Well, that's what I get for being clever with bash. I've removed the magical > "unset" for loop and am now unsetting every URL explicitly. "It's a big global bag of strings. What could go wrong?" -- 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: 8 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Sat, 11 Nov 2017 00:24:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8456 ) Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. IMPALA-6148: Specifying thirdparty deps as URLs If the environment variable $IMPALA__URL is configured in impala-config-branch.sh or impala-config-local, for a thirdparty dependency, use that to download it instead of the s3://native-toolchain bucket. This makes testing against arbitrary versions of the dependencies easier. I did a little bit of refactoring while here, creating a small class for a Package to handle reading the environment variables. I also changed bootstrap_toolchain.py to use Python logging, which cleans up the output during the multi-threaded downloading. I tested this by both with customized URLs and by running the regular build (pre-review-test, without most of the slow test suites). Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422 Reviewed-on: http://gerrit.cloudera.org:8080/8456 Reviewed-by: Joe McDonnell Tested-by: Impala Public Jenkins --- M bin/bootstrap_toolchain.py M bin/impala-config.sh 2 files changed, 138 insertions(+), 57 deletions(-) Approvals: Joe McDonnell: Looks good to me, approved Impala Public Jenkins: Verified -- 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: merged Gerrit-Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422 Gerrit-Change-Number: 8456 Gerrit-PatchSet: 8 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8456 ) Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. Patch Set 7: Verified+1 -- 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: 7 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Fri, 10 Nov 2017 02:42:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8456 ) Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1457/ -- 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: 7 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Thu, 09 Nov 2017 23:23:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/8456 ) Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. Patch Set 6: Code-Review+2 -- 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: 6 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Thu, 09 Nov 2017 23:22:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/8456 ) Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. Patch Set 7: Code-Review+2 -- 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: 7 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Thu, 09 Nov 2017 23:22:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
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 6: Well, that's what I get for being clever with bash. I've removed the magical "unset" for loop and am now unsetting every URL explicitly. -- 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: 6 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Thu, 09 Nov 2017 18:18:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Hello David Knupp, Joe McDonnell, Zach Amsden, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8456 to look at the new patch set (#6). Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. IMPALA-6148: Specifying thirdparty deps as URLs If the environment variable $IMPALA__URL is configured in impala-config-branch.sh or impala-config-local, for a thirdparty dependency, use that to download it instead of the s3://native-toolchain bucket. This makes testing against arbitrary versions of the dependencies easier. I did a little bit of refactoring while here, creating a small class for a Package to handle reading the environment variables. I also changed bootstrap_toolchain.py to use Python logging, which cleans up the output during the multi-threaded downloading. I tested this by both with customized URLs and by running the regular build (pre-review-test, without most of the slow test suites). Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422 --- M bin/bootstrap_toolchain.py M bin/impala-config.sh 2 files changed, 138 insertions(+), 57 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/8456/6 -- 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: newpatchset Gerrit-Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422 Gerrit-Change-Number: 8456 Gerrit-PatchSet: 6 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
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 5: (1 comment) Thanks for starting that gerrit-verify-dryrun job Joe -- I always forget that. http://gerrit.cloudera.org:8080/#/c/8456/5/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/8456/5/bin/impala-config.sh@142 PS5, Line 142: unset $var Looks like this is the cause for the gerrit-verify-dryrun failure at https://jenkins.impala.io/job/all-build-options-ub1604/145/console. That Jenkins job relies on a variable called IMPALA_REPO_URL. -- 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: 5 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Thu, 09 Nov 2017 16:31:13 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8456 ) Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. Patch Set 5: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1452/ -- 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: 5 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Thu, 09 Nov 2017 04:20:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8456 ) Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. Patch Set 5: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1452/ -- 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: 5 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Thu, 09 Nov 2017 00:50:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Joe McDonnell has removed a vote on this change. Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. Removed Verified+1 by Joe McDonnell -- 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: deleteVote Gerrit-Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422 Gerrit-Change-Number: 8456 Gerrit-PatchSet: 5 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/8456 ) Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. Patch Set 5: Verified+1 Code-Review+2 Rebase, carry +2. -- 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: 5 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Wed, 08 Nov 2017 23:03:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
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 4: Code-Review+2 (1 comment) > Patch Set 4: > > (1 comment) 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@112 PS2, Line 112: if re.search(k, release): > The 57 times is why we're adding the cache. It was spamming the log, mostly Got it. Thanks for pointing that line out. It does seem to me that baking in platform as an attribute of Package fits the intention of the class -- perhaps with a default value, but override-able such that L240 might look like: Package("kudu", kudu_version, "centos7") But I get not wanting to get into excessive refactoring of existing code, and for now not spamming the log is an improvement, so I think what you have here is fine. -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Wed, 08 Nov 2017 21:58:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
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: (1 comment) 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@112 PS2, Line 112: if re.search(k, release): > This is probably an obtuse question (you can count on me for those): for an The 57 times is why we're adding the cache. It was spamming the log, mostly. (It also saves a second, which is sort of nice on a warm build.) But release can be passed in as an argument to get_platform_release_label(), which is done if Kudu isn't supported; line 240. -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Wed, 08 Nov 2017 21:09:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
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 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/8456/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8456/4//COMMIT_MSG@10 PS4, Line 10: , Nit: this comma is probably not necessary; it actually kind of makes the sentence less clear. 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@112 PS2, Line 112: if re.search(k, release): > You can't trivially cache v because it depends on the argument 'release'. This is probably an obtuse question (you can count on me for those): for any given invocation of this script, won't release always be the same thing? E.g., if I'm bootstrapping the toolchain on my dev machine, the release will only ever resolve to ubuntu14.04, no matter how many times "lsb_release -irs" gets called (57 times, it turns out -- fewer if parts of the toolchain are already there.) -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Wed, 08 Nov 2017 20:54:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
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__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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Wed, 08 Nov 2017 18:10:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Hello David Knupp, Joe McDonnell, Zach Amsden, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8456 to look at the new patch set (#4). Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. IMPALA-6148: Specifying thirdparty deps as URLs If the environment variable $IMPALA__URL is configured in impala-config-branch.sh or impala-config-local, for a thirdparty dependency, use that to download it instead of the s3://native-toolchain bucket. This makes testing against arbitrary versions of the dependencies easier. I did a little bit of refactoring while here, creating a small class for a Package to handle reading the environment variables. I also changed bootstrap_toolchain.py to use Python logging, which cleans up the output during the multi-threaded downloading. I tested this by both with customized URLs and by running the regular build (pre-review-test, without most of the slow test suites). Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422 --- M bin/bootstrap_toolchain.py M bin/impala-config.sh 2 files changed, 101 insertions(+), 57 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/8456/4 -- 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: newpatchset Gerrit-Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422 Gerrit-Change-Number: 8456 Gerrit-PatchSet: 4 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
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: (1 comment) 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@112 PS2, Line 112: return v > Why not cache v instead? Also, would it be appropriate to cache the value as an attribute of Package() instead? -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Tue, 07 Nov 2017 17:29:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/8456 ) Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. Patch Set 2: (1 comment) 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@112 PS2, Line 112: return v Why not cache v instead? -- 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: 2 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Tue, 07 Nov 2017 16:45:30 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Tue, 07 Nov 2017 01:59:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/8456 ) Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. Patch Set 3: (1 comment) 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__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. Instead, they must be set in impala-config-branch.sh or impala-config-local.sh. Is that what we want? One thing that we do for some environment variables crucial to the build is to have an OVERRIDE version of these variables. Sometimes we also respect the environment itself. See: DOWNLOAD_CDH_COMPONENTS HADOOP_INCLUDE_DIR_OVERRIDE HADOOP_LIB_DIR_OVERRIDE HIVE_SRC_DIR_OVERRIDE -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Tue, 07 Nov 2017 01:34:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Hello David Knupp, Joe McDonnell, Zach Amsden, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8456 to look at the new patch set (#3). Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. IMPALA-6148: Specifying thirdparty deps as URLs If the environment variable $IMPALA__URL is configured for a thirdparty dependency, use that to download it instead of the s3://native-toolchain bucket. This makes testing against arbitrary versions of the dependencies easier. I did a little bit of refactoring while here, creating a small class for a Package to handle reading the environment variables. I also changed bootstrap_toolchain.py to use Python logging, which cleans up the output during the multi-threaded downloading. I tested this by both with customized URLs and by running the regular build (pre-review-test, without most of the slow test suites). Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422 --- M bin/bootstrap_toolchain.py M bin/impala-config.sh 2 files changed, 85 insertions(+), 44 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/8456/3 -- 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: newpatchset Gerrit-Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422 Gerrit-Change-Number: 8456 Gerrit-PatchSet: 3 Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden
[Impala-ASF-CR] IMPALA-6148: Specifying thirdparty deps as URLs
Philip Zeyliger has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8456 Change subject: IMPALA-6148: Specifying thirdparty deps as URLs .. IMPALA-6148: Specifying thirdparty deps as URLs If the environment variable $IMPALA__URL is configured for a thirdparty dependency, use that to download it instead of the s3://native-toolchain bucket. This makes testing against arbitrary versions of the dependencies easier. I did a little bit of refactoring while here, creating a small class for a Package to handle reading the environment variables. I also changed bootstrap_toolchain.py to use Python logging, which cleans up the output during the multi-threaded downloading. I tested this by both with customized URLs and by running the regular build (pre-review-test, without most of the slow test suites). Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422 --- M bin/bootstrap_toolchain.py 1 file changed, 77 insertions(+), 44 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/8456/2 -- 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: newchange Gerrit-Change-Id: I4628d86022d4bd8b762313f7056d76416a58b422 Gerrit-Change-Number: 8456 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger