impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4407: Move Impala setup procedures to main repo
Date Thu, 10 Aug 2017 14:58:20 GMT
Jim Apple has posted comments on this change.

Change subject: IMPALA-4407: Move Impala setup procedures to main repo

Patch Set 3:

File bin/

PS3, Line 21: system
            : # configurations, so it is best to run this in a fresh install. It also sets
up the
            : # .bashrc of the calling user with some environment variables
> I know that the impala-setup rep didn't do this, but do you think it would 

Line 57:      maven ninja-build ntp ntpdate python-dev python-setuptools postgresql
> Please add the following too:

PS3, Line 90: # IMPALA-3932, IMPALA-3926
            : SET_LD_LIBRARY_PATH='export LD_LIBRARY_PATH=/usr/lib/x86_64-linux-gnu/${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}'
            : echo "$SET_LD_LIBRARY_PATH" >> ~/.bashrc
            : eval "$SET_LD_LIBRARY_PATH"
> 1. Is this needed for Ubuntu 14? I don't do this on my personal Ubuntu 14 s
1. Fixed

2. Agreed. While we could grep .bashrc for this, that will sometimes show it as being already
done when, in fact, changes that are below that line in the .bashrc override it. I have moved
the output location to, but I think the same applies.

PS3, Line 96: sudo -u postgres psql -c "CREATE ROLE hiveuser LOGIN PASSWORD 'password';" postgres
> If you run this script twice, this line always fails. Could this be enhance

PS3, Line 115: echo "NoHostAuthenticationForLocalhost yes" >> ~/.ssh/config
> It might be polite to have a prompt for this since it's a security change.
Added one at the top

PS3, Line 118: echo " $(hostname -s) $(hostname)" | sudo tee -a /etc/hosts
             : sudo sed -i 's/' /etc/hosts
> This will keep appending to /etc/hosts if you run the script more than once
I agree. See above about .bashrc, though

PS3, Line 124: echo "* - nofile 1048576" | sudo tee -a /etc/security/limits.conf
> 1. This keeps appending to limits.conf
1. See above
2. Added #TODO. It can't just be $(whoami), because it might need that for postgres or root.

PS3, Line 127: export IMPALA_HOME="$(pwd)"
> What do you think about setting this in .bashrc also?

PS3, Line 131: git clone
             : ln -s impala-lzo Impala-lzo
             : git clone
> This fails if you run the script twice. Could you add -d tests here similar

PS3, Line 139: if [[ $VERSION = "14.04" ]]
             : then
             :   unset LD_LIBRARY_PATH
             : fi
> Oh, this sort of answers my question above. What about when the user create

PS3, Line 145: time -p ./ -noclean -format -testdata
> I'm torn about this, because it takes hours. Maybe consider ./ -
Changed so it doesn't run tests - I think it might be confusing otherwise to users who think
they have bootstrapped but then can't run a test because the data isn't loaded, especially
because loading data is somewhat uncommon in my experience as a step to get a working dev
environment (among other open source projects).this

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I728abfa806ecd9461dfb443278c2a464714d984c
Gerrit-PatchSet: 3
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: Sailesh Mukil <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message