You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zach Amsden (Code Review)" <ge...@cloudera.org> on 2017/08/04 01:02:15 UTC

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

Zach Amsden has uploaded a new change for review.

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................

IMPALA-5764: Allow overriding packaged components

For allowing multiple different distributions to build against the same
codebase, allow overriding various environment variables as well as what
packages to download into the toolchain.  This allows multiple sets of
packaged components to exist in the toolchain and testdata concurrently,
and to easily swap between them using environment overrides.

Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
---
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M testdata/cluster/admin
3 files changed, 56 insertions(+), 43 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/7581/1//COMMIT_MSG
Commit Message:

Line 9: For allowing multiple different distributions to build against the same
> Maybe briefly mention the relevant environment variables so it's more disco
Done


Line 14: 
> can you comment on how other 'packages' can be set up? e.g. what's required
Yeah, these two changes are somewhat intertwined.  I'll put most of the explanation in README.md rather than the changelog which nobody will read ;)


http://gerrit.cloudera.org:8080/#/c/7581/1/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:

Line 39: HOST = "https://native-toolchain.s3.amazonaws.com/build"
> It might make sense to also allow configuring this URL, so it can be pointe
I considered it, but it's easy to open a slippery slope to 'Everything is an environment variable!' and this gets used for both the components and the toolchain with different paths, so I refrained from letting the change snowball.


PS1, Line 340: llama,
> nit: remove llama (though not llama-minikdc)
Done


PS1, Line 341: minikidc
> typo: minikdc
Done.  Wait - that wasn't even my tyop!  Will fix anyway.


http://gerrit.cloudera.org:8080/#/c/7581/1/bin/impala-config.sh
File bin/impala-config.sh:

PS1, Line 131: cdh
> Keeping it as cdh_components kind-of makes sense to me as long as we're put
My intention was to leave this alone so that the existing workflows for both external and internal developers continue to work.

The override I had in mind for C6 looks something like:

PACKAGED_COMPONENTS_NAME=cdh6
PACKAGED_COMPONENTS_PATH="/cdh6_components/"
PACKAGED_COMPONENTS="avro,hadoop,hbase,hive,sentry"

Then we can have both C5 & C6 builder jobs run separately and deposit components where they won't conflict.  Also note we can pull in avro and drop llama-minikdc (or also pull in hbase-minikdc, if someone is brave enough to set that up)

And of course external thirdparty components are pretty easy to add as well, but with this arrangement (components override toolchain, as I want to happen for avro), one might argue for adding the other Apache components directly into the toolchain and letting the build override them.

But one step at a time, this should suffice for now.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 1:

Ping?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7581/1/bin/impala-config.sh
File bin/impala-config.sh:

PS1, Line 131: cdh
> Keeping it as cdh_components kind-of makes sense to me as long as we're put
I see, if that's the use case that makes sense. It might be helpful to have a comment indicating how you might also include X_components.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has abandoned this change. ( http://gerrit.cloudera.org:8080/7581 )

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Abandoned

Abandoning ancient thing.  Now that we're almost settled with the build / toolchain / branch structure, we'll do something better.
-- 
To view, visit http://gerrit.cloudera.org:8080/7581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-Change-Number: 7581
Gerrit-PatchSet: 2
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7581/1//COMMIT_MSG
Commit Message:

Line 9: For allowing multiple different distributions to build against the same
Maybe briefly mention the relevant environment variables so it's more discoverable to people looking at the commit message only.


http://gerrit.cloudera.org:8080/#/c/7581/1/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:

Line 39: HOST = "https://native-toolchain.s3.amazonaws.com/build"
It might make sense to also allow configuring this URL, so it can be pointed to an alternative source of the components.


http://gerrit.cloudera.org:8080/#/c/7581/1/bin/impala-config.sh
File bin/impala-config.sh:

PS1, Line 131: cdh
> is it too painful to change this string? i guess renaming would break exist
Keeping it as cdh_components kind-of makes sense to me as long as we're putting CDH components in there, to allow parallel sets of components (e.g. apache_components).

Or I guess maybe it could also make sense to have hadoop_components/ minicluster_components or something like that with everything under it, distinguished by version numbers.

I don't feel strongly either way.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 2:

What's the next step on this one?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 1:

needs rebase against merged changes, coming presently

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7581/2/bin/impala-config.sh
File bin/impala-config.sh:

Line 180: export DOWNLOAD_PACKAGED_COMPONENTS=${DOWNLOAD_PACKAGED_COMPONENTS-"$NO_THIRDPARTY"}
> Technically this "breaks" the C6 build, but because the cdh6.x version of I
This is the code review tool for Apache Impala. It is fine to add features to Impala to help other software that works with Impala, including proprietary products.

However, please try to explain enough in your discussion here so that a person who does not work at Cloudera can help review your code and understand all the jargon like "pushed from CDH5" and "cauldron".


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7581/2/bin/impala-config.sh
File bin/impala-config.sh:

Line 180: export DOWNLOAD_PACKAGED_COMPONENTS=${DOWNLOAD_PACKAGED_COMPONENTS-"$NO_THIRDPARTY"}
> The proprietary stuff is what I'm actually trying to get rid of with this c
Got it. However, my comment was trying to address the jargon of your comment, not the goal of the patch. I don't think there is any problem with the goal of the patch.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7581/1//COMMIT_MSG
Commit Message:

Line 9: For allowing multiple different distributions to build against the same
> Done
Did you forget to push ps2?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7581/1//COMMIT_MSG
Commit Message:

Line 14: 
can you comment on how other 'packages' can be set up? e.g. what's required, and in what form. It might be useful to leave a brief description here and start a wiki page on the topic.


http://gerrit.cloudera.org:8080/#/c/7581/1/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:

PS1, Line 340: llama,
nit: remove llama (though not llama-minikdc)


PS1, Line 341: minikidc
typo: minikdc


http://gerrit.cloudera.org:8080/#/c/7581/1/bin/impala-config.sh
File bin/impala-config.sh:

PS1, Line 131: cdh
is it too painful to change this string? i guess renaming would break existing dev environments? maybe it's ok if we just force folks to bootstrap again. It'd be nice to remove 'cdh'.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................

IMPALA-5764: Allow overriding packaged components

For allowing multiple different distributions to build against the same
codebase, allow overriding various environment variables as well as what
packages to download into the toolchain.  This allows multiple sets of
packaged components to exist in the toolchain and testdata concurrently,
and to easily swap between them using environment overrides.

For example, one could specify overrides to packages and disambiguate
the resulting database paths:

PACKAGED_COMPONENTS_NAME=apache_components
PACKAGED_COMPONENTS_PATH='/apache_bucket/'
PACKAGED_COMPONENTS=avro,hadoop,hbase,hive,sentry,parquet
IMPALA_AVRO_VERSION=2.0.2-apache-local
IMPALA_HADOOP_VERSION=3.0.0-apache-experimental
IMPALA_HBASE_VERSION=1.2.0-apache-SNAPSHOT
IMPALA_HIVE_VERSION=1.1.0-apache-SNAPSHOT
IMPALA_SENTRY_VERSION=1.5.1-apache-SNAPSHOT
IMPALA_PARQUET_VERSION=1.5.0-apache-SNAPSHOT

And these packages would be downloaded from the '/apache_components/'
S3 bucket into the toolchain's apache_componenets directory.

Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
---
M README.md
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M testdata/cluster/admin
4 files changed, 61 insertions(+), 49 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 2:

Need to get back to this now that I have smokes fixed.  I imagine I'll want to do things a little bit differently to avoid breaking the various integration jobs.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7581/2/bin/impala-config.sh
File bin/impala-config.sh:

Line 180: export DOWNLOAD_PACKAGED_COMPONENTS=${DOWNLOAD_PACKAGED_COMPONENTS-"$NO_THIRDPARTY"}
> This is the code review tool for Apache Impala. It is fine to add features 
The proprietary stuff is what I'm actually trying to get rid of with this change.  Needless to say, downstream things that are affected by this will need to be fixed downstream; right now I'm unaware of any major problems there that cannot easily be fixed.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 2:

(4 comments)

A couple of minor nits.  Also, let me make sure I didn't accidentally introduce typos during edits and that the script and data load still work.

http://gerrit.cloudera.org:8080/#/c/7581/2/README.md
File README.md:

Line 69: | PACKAGED_COMPONENTS_NAME | "cdh" | Identifier used to uniquify paths for potentially incompatible component builds. |
This doesn't currently match the "cdh5" label used in the shell script.


http://gerrit.cloudera.org:8080/#/c/7581/2/bin/impala-config.sh
File bin/impala-config.sh:

Line 180: export DOWNLOAD_PACKAGED_COMPONENTS=${DOWNLOAD_PACKAGED_COMPONENTS-"$NO_THIRDPARTY"}
Technically this "breaks" the C6 build, but because the cdh6.x version of Impala is pushed right now from CDH5 with a thirdparty directory, this is still working.  Will need to update the cauldron build to fix this.


Line 361:   export PACKAGED_COMPONENTS_HOME="$IMPALA_TOOLCHAIN/$PACKAGED_COMPONENTS_NAME"
This changes the download directory for the CDH pieces of the toolchain; I presume that is okay to do, might result in duplicate copies but nothing should break because of this.


Line 382: export MINI_DFS_BASE_DATA_DIR="$IMPALA_HOME/${PACKAGED_COMPONENTS_NAME}-hdfs-data"
This forces a data reload - unless there is any objection, I'm not going to try to change that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

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

Change subject: IMPALA-5764: Allow overriding packaged components
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7581/1//COMMIT_MSG
Commit Message:

Line 14: 
> can you comment on how other 'packages' can be set up? e.g. what's required
Nevermind, it looks like you started this in a separate review. That was lower down in my inbox :)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes