Jim Apple has posted comments on this change.
Change subject: IMPALA-4407: Move Impala setup procedures to main repo
Patch Set 2:
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
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:
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?
PS2, Line 70: grep amazon
> You should check for the existence of dmicode before calling it, i.e. if wh
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_LIB
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'
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'
To view, visit http://gerrit.cloudera.org:8080/7587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: Jim Apple <jbapple-imp...@apache.org>
Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>