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 Sun, 06 Aug 2017 22:49:57 GMT
Jim Apple has posted comments on this change.

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

Patch Set 2:

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

Line 30: # exploration mode.
> Do we have an umbrella Jenkins that we use to test changes made to this scr
Since Jenkins jobs are configured away from this script, I'd prefer to keep the comments separate
to avoid false dependencies: if someone deletes a Jenkins job, I don't want the comment here
to grow stale.

In any case, the only such job is an experimental one:

PS2, Line 34: grep "Ubuntu"
> nit: "grep -q" prevents printing the match.
I'm OK with it printing. For a batch bash script, and one that runs the tests for a project
like Impala with a dense history of flaky tests, flaky bootstrapping, and flaky builds, I
think verbosity in the output is a virtue.

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 cou
I thin that would actually add lines and complexity.

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 wh

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_LIB

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'

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' 

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