You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2016/10/11 19:11:57 UTC

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup. 

Tim Armstrong has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4685

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................

IMPALA-4259: build Impala without any test cluster setup.

The main outcome of this change is to avoid making unnecessary
modification to the Impala or other source trees when we don't need the
test cluster.

To achieve that, this refactors the script to make the flow easier
to understand and makes it more consistent which build steps are
executed in which modes.

Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
---
M bin/clean.sh
M buildall.sh
2 files changed, 178 insertions(+), 144 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/4685/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 7: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4685/5/buildall.sh
File buildall.sh:

Line 263:   NEED_TEST_CLUSTER=1
> NEED_MINICLUSTER?
Done


Line 316:   pushd "${IMPALA_FE_DIR}"
> I'm a little confused by having a "fe" cmake target that we are using in bu
Done. Also made the fe/ext-data-source targets use mvn-quiet.


-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4685/1/buildall.sh
File buildall.sh:

PS1, Line 424: start_test_cluster
> I think that would be helpful in many cases if it brought up an Impala clus
I think the asymmetry is partly what bugs me. Probably good to remove the start for Kerberos, and then add a flag for starting up that only gets evalulated if TESTDATA_ACTION and TESTS_ACTION are both false.


-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#3).

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................

IMPALA-4259: build Impala without any test cluster setup.

The main outcome of this change is to avoid making unnecessary
modification to the Impala or other source trees when we don't need the
test cluster.

To achieve that, this refactors the script to make the flow easier
to understand and makes it more consistent which build steps are
executed in which modes.

Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
---
M bin/clean.sh
M buildall.sh
2 files changed, 174 insertions(+), 144 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/4685/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello David Knupp,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4685

to look at the new patch set (#6).

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................

IMPALA-4259: build Impala without any test cluster setup.

The main outcome of this change is to avoid making unnecessary
modification to the Impala or other source trees when we don't need the
test cluster.

To achieve that, this refactors the script to make the flow easier
to understand and makes it more consistent which build steps are
executed in which modes.

Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
---
M bin/clean.sh
M buildall.sh
M ext-data-source/CMakeLists.txt
M fe/CMakeLists.txt
4 files changed, 179 insertions(+), 163 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/4685/6
-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 6: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4685/6/buildall.sh
File buildall.sh:

Line 311:   "${MAKE_CMD}" ext-data-source fe
> We don't but otherwise it's hard to see that this actually builds both arti
wfm


-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4685/6/buildall.sh
File buildall.sh:

Line 311:   "${MAKE_CMD}" ext-data-source fe
> do we need the "ext-data-source" part? the fe cmake target depends on it
We don't but otherwise it's hard to see that this actually builds both artifacts without scrounging through CMake files (I was thinking of the ext-data-source jar as a separate build artifact from the frontend).


-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 7: Code-Review+2

The merge failed because the merge jump runs buildall.sh from a different directory. Calling make without changing directories broke that.

-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4685/1/buildall.sh
File buildall.sh:

PS1, Line 424: start_test_cluster
> I think the asymmetry is partly what bugs me. Probably good to remove the s
That makes sense. I added the flag and also changed the kerberos behaviour to just bail out if it is asked to do something that it can't. I think this is a significant improvement.


-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello David Knupp,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4685

to look at the new patch set (#5).

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................

IMPALA-4259: build Impala without any test cluster setup.

The main outcome of this change is to avoid making unnecessary
modification to the Impala or other source trees when we don't need the
test cluster.

To achieve that, this refactors the script to make the flow easier
to understand and makes it more consistent which build steps are
executed in which modes.

Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
---
M bin/clean.sh
M buildall.sh
2 files changed, 182 insertions(+), 159 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/4685/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#2).

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................

IMPALA-4259: build Impala without any test cluster setup.

The main outcome of this change is to avoid making unnecessary
modification to the Impala or other source trees when we don't need the
test cluster.

To achieve that, this refactors the script to make the flow easier
to understand and makes it more consistent which build steps are
executed in which modes.

Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
---
M bin/clean.sh
M buildall.sh
2 files changed, 182 insertions(+), 144 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/4685/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4685/1/buildall.sh
File buildall.sh:

PS1, Line 386: 5
> No idea, I just moved the preexisting code. 
If a more meaningful test could be substituted, that would be great. If not, then I think I agree -- removing it would probably be better.


PS1, Line 424: start_test_cluster
> It is a little weird. I guess because the test or data loading scripts want
I don't want to hold up this review, since this is a clear improvement on what exists, so consider this speculative. It would be nice if the outcome of the buildall.sh script were simply a running Impala cluster, whether or not data gets loaded or tests get run -- e.g., perhaps by adding a line to call start-impala-cluster.py with no params if neither TESTDATA_ACTION nor TESTS_ACTION are true.

In fact, you could uncouple that line from the "if [[ ${IMPALA_KERBERIZE} -eq 1 ]]" block, and add it in after the fact. In other words:


  if [[ ${TESTDATA_ACTION} -eq 0  &&  ${TESTS_ACTION} -eq 0 ]]; then
    "${IMPALA_HOME}/bin/start-impala-cluster.py"
  fi


(My bash still kinda sucks, so that might not be the exact right syntax.)

This way, we won't wind up with this one oddball condition whereby if we run non-kerberized, but without loading up data or running tests, we don't even bother starting up Impala. Otherwise, the starting of Impala will be handled by the other scripts. 

(FWIW, from a cursory glance at those other scripts, it appears that they each employ some basic logic to determine the startup params to use for Impala (e.g., log_dir location, cluster_size, impalad_args, etc. etc.), and then independently call start-impala-cluster.py. So it actually gets called more than once if you are both loading data and then also running tests.)


-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello David Knupp, Internal Jenkins, Alex Behm,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4685

to look at the new patch set (#7).

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................

IMPALA-4259: build Impala without any test cluster setup.

The main outcome of this change is to avoid making unnecessary
modification to the Impala or other source trees when we don't need the
test cluster.

To achieve that, this refactors the script to make the flow easier
to understand and makes it more consistent which build steps are
executed in which modes.

Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
---
M bin/clean.sh
M buildall.sh
M ext-data-source/CMakeLists.txt
M fe/CMakeLists.txt
4 files changed, 181 insertions(+), 163 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/4685/7
-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4685/1/buildall.sh
File buildall.sh:

PS1, Line 386: 5
Where does the 5 come from? E.g., in my dev environment, there are 996 items inside of test-warehouse.


PS1, Line 424: start_test_cluster
This only just occurred to me, but is it weird that buildall.sh doesn't actually start impala if no tests are run, since calling start-impala-cluster.py seems to be delegated to run-all-tests.sh and create-load-data.sh (except in the case of a kerberized setup)? This was true even before this patch.

It seems a bit odd to be me at any rate. Is it by design? Or oversight? Should start_test_cluster be called start_hadoop_cluster, or something similar, to more clearly indicate that what is being started are all of the other non-impala hadoop components.


-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


IMPALA-4259: build Impala without any test cluster setup.

The main outcome of this change is to avoid making unnecessary
modification to the Impala or other source trees when we don't need the
test cluster.

To achieve that, this refactors the script to make the flow easier
to understand and makes it more consistent which build steps are
executed in which modes.

Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Reviewed-on: http://gerrit.cloudera.org:8080/4685
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Internal Jenkins
---
M bin/clean.sh
M buildall.sh
M ext-data-source/CMakeLists.txt
M fe/CMakeLists.txt
4 files changed, 181 insertions(+), 163 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 4: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4685/1/buildall.sh
File buildall.sh:

PS1, Line 386: 5
> If a more meaningful test could be substituted, that would be great. If not
Done


PS1, Line 424: start_test_cluster
> I don't want to hold up this review, since this is a clear improvement on w
I think that would be helpful in many cases if it brought up an Impala cluster. I run ./buildall.sh -debug -noclean -notests -skiptests -ninja && start-impala-cluster.py all the time.

The problem that I see is that there are a number of consumers of this script that assume passing -notests is sufficient to only build Impala and not try to start a test cluster. Part of what I was hoping to achieve is to do even less in that case - avoid creating cluster configs, etc.

Do you think it would be helpful to add a new flag to start the cluster? E.g. -start_impala_cluster?

I think we could definitely remove the Kerberos special-case - no reason it should start a cluster then when the other modes don't.


-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4685/6/buildall.sh
File buildall.sh:

Line 311:   "${MAKE_CMD}" ext-data-source fe
do we need the "ext-data-source" part? the fe cmake target depends on it


-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#4).

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................

IMPALA-4259: build Impala without any test cluster setup.

The main outcome of this change is to avoid making unnecessary
modification to the Impala or other source trees when we don't need the
test cluster.

To achieve that, this refactors the script to make the flow easier
to understand and makes it more consistent which build steps are
executed in which modes.

Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
---
M bin/clean.sh
M buildall.sh
2 files changed, 182 insertions(+), 159 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/4685/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 5:

(2 comments)

So much better

http://gerrit.cloudera.org:8080/#/c/4685/5/buildall.sh
File buildall.sh:

Line 263:   NEED_TEST_CLUSTER=1
NEED_MINICLUSTER?


Line 316:   pushd "${IMPALA_FE_DIR}"
I'm a little confused by having a "fe" cmake target that we are using in build_fe() but not here. Looks like building the fe with make would take care of building its dependencies also, so maybe we can simplify the flow here?


-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 6: Verified-1

Build failed: http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/280/

-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4259: build Impala without any test cluster setup.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4259: build Impala without any test cluster setup.
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4685/1/buildall.sh
File buildall.sh:

PS1, Line 386: 5
> Where does the 5 come from? E.g., in my dev environment, there are 996 item
No idea, I just moved the preexisting code. 

I kind of feel like we should just remove this check since I'm not sure how useful it really is.


PS1, Line 424: start_test_cluster
> This only just occurred to me, but is it weird that buildall.sh doesn't act
It is a little weird. I guess because the test or data loading scripts want to restart Impala anyway.

I renamed to start_test_cluster_dependencies - hopefully that's clearer.


-- 
To view, visit http://gerrit.cloudera.org:8080/4685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I429da7bc6681b16c07fe58bb3efac6d1a8579137
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes