Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/10748 )
Change subject: IMPALA-7180: Pin Impala CDH dependencies ...................................................................... Patch Set 2: (14 comments) I left some comments but I'm don't know how to validate the changes in the .pom files. http://gerrit.cloudera.org:8080/#/c/10748/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10748/2//COMMIT_MSG@9 PS2, Line 9: For profile=3 Without context it's hard to understand what that means. Can you add a sentence to introduce the meaning? http://gerrit.cloudera.org:8080/#/c/10748/2//COMMIT_MSG@11 PS2, Line 11: between extra word http://gerrit.cloudera.org:8080/#/c/10748/2//COMMIT_MSG@10 PS2, Line 10: Maven : reposito Maybe reword to "Maven artifacts" or "Maven dependencies"? Or are we really storing the repository in S3? If so, how does it get there? http://gerrit.cloudera.org:8080/#/c/10748/2//COMMIT_MSG@16 PS2, Line 16: https://repositories.cloudera.com Isn't this repository.cloudera.com ? http://gerrit.cloudera.org:8080/#/c/10748/2//COMMIT_MSG@19 PS2, Line 19: global global to what? Maybe "unique build number"? http://gerrit.cloudera.org:8080/#/c/10748/2//COMMIT_MSG@28 PS2, Line 28: This patch also fixes dependency issue in Hadoop that transitively "a dependency issue" or "dependency issues"? http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py File bin/bootstrap_toolchain.py: http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py@49 PS2, Line 49: HOST = "https://native-toolchain.s3.amazonaws.com/build"; rename to distinguish from CDH_HOST. http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py@361 PS2, Line 361: def download_cdh_components(toolchain_root, cdh_components, download_path_prefix): This should have a better name, e.g. URL prefix. Otherwise it's not clear whether it's a local path or a URL. It also should be included in the method's comment. http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py@395 PS2, Line 395: the CDH components (i.e. hadoop, hbase, hive, llama, llama-minikidc and sentry) into the Update the comment to include which one gets downloaded from where. http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py@414 PS2, Line 414: The checks below fail, even when DOWNLOAD_CDH_COMPONENTS is false. Should they be moved into the block in L440? http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py@415 PS2, Line 415: if not os.environ.get("CDH_HOST"): if not "CDH_HOST" in os.environ: Seems slightly more idiomatic http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py@420 PS2, Line 420: if not os.environ.get("CDH_BUILD_NUMBER"): Both checks have the same effect, make them one check: if not ... or not ...: http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py@449 PS2, Line 449: cdh_components = map(Package, ["llama-minikdc"]) Simplify to [Package(...)] if it's only one. Also add a comment why this one is treated differently. http://gerrit.cloudera.org:8080/#/c/10748/2/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/10748/2/bin/impala-config.sh@191 PS2, Line 191: : ${CDH_HOST:=native-toolchain.s3.amazonaws.com} CDH_DEPENDENCY_HOST? CDH_DOWNLOAD_HOST? Could this conceivably by anything but a CDH host? If so, maybe rename it to HADOOP_DEPENDENCY_HOST? -- To view, visit http://gerrit.cloudera.org:8080/10748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I66c0dcb8abdd0d187490a761f129cda3b3500990 Gerrit-Change-Number: 10748 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Wed, 20 Jun 2018 21:34:35 +0000 Gerrit-HasComments: Yes
