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 <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>
Gerrit-HasComments: Yes

Reply via email to