impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <ger...@cloudera.org>
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:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/7587/3/bin/bootstrap_development.sh
File bin/bootstrap_development.sh:

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 
Done


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


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 impala-config.local.sh, 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
Done


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 "127.0.0.1 $(hostname -s) $(hostname)" | sudo tee -a /etc/hosts
             : sudo sed -i 's/127.0.1.1/127.0.0.1/g' /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?
Done


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


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
Fixed


PS3, Line 145: time -p ./buildall.sh -noclean -format -testdata
> I'm torn about this, because it takes hours. Maybe consider ./buildall.sh -
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 http://gerrit.cloudera.org:8080/7587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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

Mime
View raw message