Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/13432 )
Change subject: WIP: IMPALA-8586: Support download URLs for CDP ...................................................................... Patch Set 1: (7 comments) I'm pretty sure extending this to the ordinary toolchain is doable. I had an idea of how that would work. If this is an ok direction, I'll take that on. http://gerrit.cloudera.org:8080/#/c/13432/1/bin/bootstrap_toolchain.py File bin/bootstrap_toolchain.py: http://gerrit.cloudera.org:8080/#/c/13432/1/bin/bootstrap_toolchain.py@239 PS1, Line 239: cdp_components > since we are at, I think having a separate cdp_components doesn't make muc I don't have a strong opinion on this. I think I would come down on the side of leaving it the way it is. I like that the URLs/directory names are self-documenting. The tarball names and version scheme are different, so they are not interchangeable. I think keeping some distinction between the CDH and CDP downloads is useful for debugging things. http://gerrit.cloudera.org:8080/#/c/13432/1/bin/bootstrap_toolchain.py@536 PS1, Line 536: USE_CDP_HIVE > Should we change this to something more generic like USE_CDP_COMPONENTS if Yes, we should change this env var name. USE_CDP_COMPONENTS seems fine. I might put that in this change. It requires some coordination with existing jobs that already use USE_CDP_HIVE. http://gerrit.cloudera.org:8080/#/c/13432/1/bin/bootstrap_toolchain.py@604 PS1, Line 604: CDP_COMPONENTS_HOME > do you think its better to get rid of CDP_COMPONENT_HOME and just have a CD My feelings are similar to the URL point. It is self-documenting to include cdp/cdh in the directory name. I can "ls -l $IMPALA_TOOLCHAIN" and see what is the latest cdh or cdp component directory. It makes the CLASSPATH a bit clearer about what it is using. http://gerrit.cloudera.org:8080/#/c/13432/1/bin/bootstrap_toolchain.py@648 PS1, Line 648: download_path_prefix > can we move this to the CdhComponent class? Right now, llama-minikdc sits in a location in the s3 bucket that doesn't include the build number. It almost never changes, so we don't upload it with each change of the build number. There might be a way to make this fit; it is a special case. http://gerrit.cloudera.org:8080/#/c/13432/1/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/13432/1/bin/impala-config.sh@167 PS1, Line 167: CDH_HBASE_VERSION > Its probably easier on eyes if we have all CDH_*VERSION together and all CD That makes sense. I will fix that. http://gerrit.cloudera.org:8080/#/c/13432/1/bin/impala-config.sh@217 PS1, Line 217: export IMPALA_HIVE_URL=$CDP_HIVE_URL > can we use this syntax for all if blocks below Yes, I definitely meant to find a better syntax. This sounds much better. http://gerrit.cloudera.org:8080/#/c/13432/1/bin/impala-config.sh@359 PS1, Line 359: CDP_COMPONENTS_HOME > if we could get rid of have a separate CDP_COMPONENTS_HOME, we can make the I think if we can get down to one if/else, then that would be good. There are still some variations in directory names (apache-hive-{version}-bin vs hive-{version}), and tez vs no-tez. -- 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: 1 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Thu, 30 May 2019 00:48:37 +0000 Gerrit-HasComments: Yes
