impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <>
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:


Thank you for creating this!
File bin/

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?

Consider using something like the following. That way we'll maintain LD_LIBRARY_PATH even
if the user changes it after our script runs. 


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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message