Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10748 )
Change subject: IMPALA-7180: Pin Impala CDH dependencies ...................................................................... Patch Set 5: Code-Review+1 (7 comments) I think this is fine. I have some nits, but nothing earth-shattering. When the CDH_BUILD_NUMBER variable changes upstream, will people need to re-build their miniclusters? I don't know if we're going to like have the build numbers in impala-config.sh. On one hand, it's reproducible, but on the other, it may have annoying noisy commits and so on. http://gerrit.cloudera.org:8080/#/c/10748/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10748/5//COMMIT_MSG@15 PS5, Line 15: Pin the CDH dependencies by storing only the CDH tarballs in S3. nit: lowercase pin http://gerrit.cloudera.org:8080/#/c/10748/5//COMMIT_MSG@16 PS5, Line 16: The Maven repository will still use https://repository.cloudera.com. nit:", so" across this and the next line. http://gerrit.cloudera.org:8080/#/c/10748/5//COMMIT_MSG@28 PS5, Line 28: This patch also fixes dependency issues in Hadoop that transitively Let's be explicit about json-smart in here. Also reminds me of IMPALA-7120. If this is a bug in the CDH version of Hadoop we're picking up, can you see if it's easily fixable or fileable? http://gerrit.cloudera.org:8080/#/c/10748/5/bin/bootstrap_toolchain.py File bin/bootstrap_toolchain.py: http://gerrit.cloudera.org:8080/#/c/10748/5/bin/bootstrap_toolchain.py@403 PS5, Line 403: into the directory specified by $CDH_COMPONENTS_HOME. nit: move line 403 into line 397 "following CDH components into the directory....:" so that it's clearer? http://gerrit.cloudera.org:8080/#/c/10748/5/impala-parent/pom.xml File impala-parent/pom.xml: http://gerrit.cloudera.org:8080/#/c/10748/5/impala-parent/pom.xml@148 PS5, Line 148: <id>cdh.snapshots.repo</id> We should probably rename this id to something else. Perhaps "impala.cdh.repo". Sometimes these names show up in errors and logs, and it's useful to be able to know which one it is. http://gerrit.cloudera.org:8080/#/c/10748/5/impala-parent/pom.xml@150 PS5, Line 150: <name>CDH Snapshots Repository</name> rename this too http://gerrit.cloudera.org:8080/#/c/10748/5/testdata/pom.xml File testdata/pom.xml: http://gerrit.cloudera.org:8080/#/c/10748/5/testdata/pom.xml@39 PS5, Line 39: <!-- Force json-smart dependency. Can you mention in your commit message what's up with json-smart and javax-el? -- 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 <fwij...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Thu, 21 Jun 2018 03:04:17 +0000 Gerrit-HasComments: Yes
