Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10748 )
Change subject: IMPALA-7180: Pin Impala CDH dependencies ...................................................................... Patch Set 5: (14 comments) 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 IMPALA_MI > Without context it's hard to understand what that means. Can you add a sent It's an environment variable used in Impala to switch between Hadoop 2.x and 3.x components. I'll change it to IMPALA_MINICLUSTER_PROFILE instead. 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 Yes, we are storing the whole Maven repository. Cloudera has access to the S3 native-toolchain and they will upload the CDH dependencies when we need it. http://gerrit.cloudera.org:8080/#/c/10748/2//COMMIT_MSG@11 PS2, Line 11: between > extra word Done http://gerrit.cloudera.org:8080/#/c/10748/2//COMMIT_MSG@16 PS2, Line 16: https://repository.cloudera.com. > Isn't this repository.cloudera.com ? Typo. Done. http://gerrit.cloudera.org:8080/#/c/10748/2//COMMIT_MSG@19 PS2, Line 19: unique > global to what? Maybe "unique build number"? Done http://gerrit.cloudera.org:8080/#/c/10748/2//COMMIT_MSG@28 PS2, Line 28: This patch also fixes dependency issues in Hadoop that transitively > "a dependency issue" or "dependency issues"? Should be issues. Done. 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: TOOLCHAIN_HOST = "https://native-toolchain.s3.amazonaws.com/build"; > rename to distinguish from CDH_HOST. Done http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py@361 PS2, Line 361: def download_cdh_components(toolchain_root, cdh_components, url_prefix): > This should have a better name, e.g. URL prefix. Otherwise it's not clear w Done http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py@395 PS2, Line 395: $IMPALA_TOOLCHAIN. If $DOWNLOAD_CDH_COMPONENTS is true, the presence of > Update the comment to include which one gets downloaded from where. Done 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 t Make sense. Done. http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py@415 PS2, Line 415: # Create the destination directory if necessary > if not "CDH_HOST" in os.environ: Done http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py@420 PS2, Line 420: sys.exit(1) > Both checks have the same effect, make them one check: Done http://gerrit.cloudera.org:8080/#/c/10748/2/bin/bootstrap_toolchain.py@449 PS2, Line 449: download_cdh_components(toolchain_root, cdh_components, download_path_prefix) > Simplify to [Package(...)] if it's only one. Done 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_DOWNLOAD_HOST:=native-toolchain.s3.amazonaws.com} > CDH_DEPENDENCY_HOST? CDH_DOWNLOAD_HOST? CDH_DOWNLOAD_HOST makes more sense. The host should only contain anything related to CDH. -- 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: 5 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: Thu, 21 Jun 2018 02:30:12 +0000 Gerrit-HasComments: Yes
