Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13432 )

Change subject: IMPALA-8586: Support download URLs for CDP
......................................................................


Patch Set 3:

(5 comments)

This is a big improvement. Suggested some tweaks but i think we should try to 
get this in soon.

http://gerrit.cloudera.org:8080/#/c/13432/3/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:

http://gerrit.cloudera.org:8080/#/c/13432/3/bin/bootstrap_toolchain.py@61
PS3, Line 61: #     python bootstrap_toolchain.py
Not your change, but this is incorrect, with the #! we should call it directly 
instead of using system python


http://gerrit.cloudera.org:8080/#/c/13432/3/bin/bootstrap_toolchain.py@129
PS3, Line 129: class DownloadUnpackTarball(object):
Brief class comment explaining what the abstraction is?


http://gerrit.cloudera.org:8080/#/c/13432/3/bin/bootstrap_toolchain.py@254
PS3, Line 254:     version_file = os.path.join(unpack_dir, 
"toolchain_package_version.txt")
I would be ok with removing the toolchain_package_version.txt logic and just 
assuming that if the directory is present, it's an acceptable version. I added 
it in https://gerrit.cloudera.org/#/c/2147/ with the idea that it would be 
useful for allowing for changes to compiler versions, but I don't think it 
solves that problem.


http://gerrit.cloudera.org:8080/#/c/13432/3/bin/bootstrap_toolchain.py@584
PS3, Line 584:   # The LLVM and GCC packages are the large in the toolchain 
(Kudu is handled
Garbled?


http://gerrit.cloudera.org:8080/#/c/13432/3/bin/bootstrap_toolchain.py@719
PS3, Line 719:   download_cluster_components(downloads)
Should this be download_and_unpack_tarballs or similar? Since it's also 
downloading toolchain packages.

Or it's probably simple enough you could just inline the logic instead of 
calling a separate function.



--
To view, visit http://gerrit.cloudera.org:8080/13432
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67824fd82b820e68e9f5c87939ec94ca6abadb8c
Gerrit-Change-Number: 13432
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fre...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Comment-Date: Wed, 11 Sep 2019 21:36:22 +0000
Gerrit-HasComments: Yes

Reply via email to