Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11363 )
Change subject: IMPALA-7499: build against CDH Kudu ...................................................................... Patch Set 1: (6 comments) Thanks! Looks all reasonable. See comments below. http://gerrit.cloudera.org:8080/#/c/11363/1/bin/bootstrap_toolchain.py File bin/bootstrap_toolchain.py: http://gerrit.cloudera.org:8080/#/c/11363/1/bin/bootstrap_toolchain.py@19 PS1, Line 19: # The purpose of this script is to download prebuilt binaries and jar files to satisfy the : # third-party dependencies for Impala. The script checks for the presence of IMPALA_HOME : # and IMPALA_TOOLCHAIN. IMPALA_HOME indicates that the environment is correctly setup and : # that we can deduce the version settings of the dependencies from the environment. : # IMPALA_TOOLCHAIN indicates the location where the prebuilt artifacts should be extracted : # to. If DOWNLOAD_CDH_COMPONENTS is set to true, this script will also download and extract : # the CDH components (i.e. Hadoop, Hive, HBase and Sentry) into : # CDH_COMPONENTS_HOME. Could you update this to include KUDU_IS_SUPPORTED and USE_CDH_KUDU as they're now inputs to this script. http://gerrit.cloudera.org:8080/#/c/11363/1/bin/bootstrap_toolchain.py@71 PS1, Line 71: CDH_OS_MAPPING = { add a doc here about what this maps to and from? The keys here (and in TOOLCHAIN_OS_MAPPING) are munged versions of "lsb_release -irs". You could choose to make the value a namedtuple (or other object) that includes both "toolchain" and "cdh" values, and make it a little bit more obvious that the two mappings have to be consistent. I insist on the doc. I don't insist on merging the maps. http://gerrit.cloudera.org:8080/#/c/11363/1/bin/bootstrap_toolchain.py@107 PS1, Line 107: def get_os_version(): > flake8: E302 expected 2 blank lines, found 0 Please add pydoc here? http://gerrit.cloudera.org:8080/#/c/11363/1/bin/bootstrap_toolchain.py@154 PS1, Line 154: release = get_os_version() : for k, v in CDH_OS_MAPPING.iteritems(): : if re.search(k, release): : return v This code is identical to lines 141-145. I think it probably makes sense to write a helper function that manages that. http://gerrit.cloudera.org:8080/#/c/11363/1/bin/bootstrap_toolchain.py@475 PS1, Line 475: if os.getenv("USE_CDH_KUDU") == "false": packages += map(Package, ["kudu"]) This is just: if ...: packages += Package["kudu"] http://gerrit.cloudera.org:8080/#/c/11363/1/bin/bootstrap_toolchain.py@501 PS1, Line 501: cdh_components += map(Package, ["kudu"]) This is just: cdh_components += Package(["kudu"]) -- To view, visit http://gerrit.cloudera.org:8080/11363 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6e1048438b6d09a1b38c58371d6212bb6dcc06c Gerrit-Change-Number: 11363 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Thu, 30 Aug 2018 20:31:20 +0000 Gerrit-HasComments: Yes
