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 <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