impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4407: Move Impala setup procedures to main repo
Date Sun, 06 Aug 2017 03:24:59 GMT
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-impala@apache.org>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message