Vihang Karajgaonkar 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 think the approach makes a lot of sense. I left some suggestions below for further improvements. Let me know what you think. 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 much sense since the build_number is supposed to be unique. If we get rid of cdp_components in the path, all we really need is to use the CDP_BUILD_NUMBER and CDH_BUILD_NUMBER to construct the toolchain url which is cleaner in my opinion. Thoughts? 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 we are planning to use this flag for other components as well? 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 CDH_COMPONENTS_HOME? since the version numbers are unique it should be okay to have one single directory which hopefully will simplify the cdp vs cdh components download url logic 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? 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 CDP_*VERSION together. 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 export IMPALA_HIVE_URL=${CDP_HIVE_URL-} to improve readability? 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 these environment variables much simpler by getting rid of these if .. else .. logic. -- 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: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Wed, 29 May 2019 18:32:50 +0000 Gerrit-HasComments: Yes
