Lars Volker has posted comments on this change. Change subject: IMPALA-4407: Move Impala setup procedures to main repo ......................................................................
Patch Set 2: (10 comments) Thank you for creating this! http://gerrit.cloudera.org:8080/#/c/7587/2/bin/bootstrap_development.sh File bin/bootstrap_development.sh: Line 22: # configurations, so it is best to run this in a fresh install. Can you add a comment whether it's possible to run this again, e.g. in case of failure? Line 30: # exploration mode. Do we have an umbrella Jenkins that we use to test changes made to this script? If so, can you mention it here? PS2, Line 34: grep "Ubuntu" nit: "grep -q" prevents printing the match. PS2, Line 36: echo "This script only supports Ubuntu" >&2 : exit 1 You could define a helper die() that does the echo and exit 1. Then you could rewrite these checks as [[ foo ]] || die ... PS2, Line 66: sudo service ntp restart Why is this line needed? PS2, Line 70: grep amazon You should check for the existence of dmicode before calling it, i.e. if which dmidecode >/dev/null && sudo dmidecode -s bios-version | ... also nit: grep -q PS2, Line 73: grep amazon /etc/ntp.conf : grep ubuntu /etc/ntp.conf Are these for debugging purposes? Line 88: SET_LD_LIBRARY_PATH="$SET_LD_LIBRARY_PATH:"'$LD_LIBRARY_PATH' Consider using something like the following. That way we'll maintain LD_LIBRARY_PATH even if the user changes it after our script runs. export LD_LIBRARY_PATH=/mypath${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH} See https://stackoverflow.com/questions/9631228/how-to-smart-append-ld-library-path-in-shell-when-nounset for more details. Line 90: echo "$SET_LD_LIBRARY_PATH" >> ~/.bashrc I'm a bit concerned that this does not work on zsh systems, but I also don't have a good way of making this work across shells. Should we put the config into a ~/.impala.shellrc and then detect the shell and add a sourcing command to ~/.bashrc or ~/.zshrc? Alternatively, can you document the limitation and leave a TODO at the top of the file or open a follow up JIRA? Line 108: ssh-keygen -t rsa -N '' -q -f ~/.ssh/id_rsa We should mkdir -p ~/.ssh and chmod 600 it, it looks like ssh-keygen wont' create the folder and so this will fail on desktop installs that don't have the folder yet. -- To view, visit http://gerrit.cloudera.org:8080/7587 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
