Jim Apple has posted comments on this change. Change subject: IMPALA-4407: Move Impala setup procedures to main repo ......................................................................
Patch Set 2: (10 comments) 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 Done Line 30: # exploration mode. > Do we have an umbrella Jenkins that we use to test changes made to this scr Since Jenkins jobs are configured away from this script, I'd prefer to keep the comments separate to avoid false dependencies: if someone deletes a Jenkins job, I don't want the comment here to grow stale. In any case, the only such job is an experimental one: https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/ PS2, Line 34: grep "Ubuntu" > nit: "grep -q" prevents printing the match. I'm OK with it printing. For a batch bash script, and one that runs the tests for a project like Impala with a dense history of flaky tests, flaky bootstrapping, and flaky builds, I think verbosity in the output is a virtue. 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 cou I thin that would actually add lines and complexity. PS2, Line 66: sudo service ntp restart > Why is this line needed? Done PS2, Line 70: grep amazon > You should check for the existence of dmicode before calling it, i.e. if wh Done PS2, Line 73: grep amazon /etc/ntp.conf : grep ubuntu /etc/ntp.conf > Are these for debugging purposes? yes. 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_LIB Done 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' Done 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' Done -- 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
