You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2017/03/18 00:57:41 UTC
[kudu-CR] kudu-mapreduce: tweak poms
Hello Jean-Daniel Cryans,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6418
to review the following change.
Change subject: kudu-mapreduce: tweak poms
......................................................................
kudu-mapreduce: tweak poms
This changes hadoop-client to be a provided dependency of
kudu-mapreduce, and switches kudu-mapreduce and kudu-client-tools from
using the assembly pluging to the shade plugin (like the rest of the
submodules). Normally changing a maven dependency from compile to
provided scope might be considered a breaking change, but because the
kudu-mapreduce artifact has previously not been published as a fat
jar[1], due to the configuration of the assembly plugin, the artifact
didn't include the hadoop-client classes (which I believe means there's
no binary compat risk). In the future user M/R applications (such as
kudu-client-tools) will need to explicitly add a dependency on
hadoop-client, if they use classes from it, but they should have been
doing this anyway. Going forward, kudu-mapreduce will be published as a
fat jar containing kudu-client. kudu-client-tools will be published as a
fat jar containing kudu-client and kudu-mapreduce.
[1]: https://mvnrepository.com/artifact/org.apache.kudu/kudu-mapreduce/1.2.0
Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
---
D java/assembly.xml
M java/kudu-client-tools/pom.xml
M java/kudu-mapreduce/pom.xml
3 files changed, 28 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/6418/1
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
[kudu-CR] kudu-mapreduce: tweak poms
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: kudu-mapreduce: tweak poms
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] kudu-mapreduce: tweak poms
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: kudu-mapreduce: tweak poms
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6418/1//COMMIT_MSG
Commit Message:
Line 22: fat jar containing kudu-client and kudu-mapreduce.
> kudu-mapreduce only has three compile-scope dependencies:
I think it's valuable to shade async and kudu-client because async is exposed as part of the kudu-client public API and the asynchbase client has a dependency on async. Without shading, if you want to write to both HBase (using asynchbase) and Kudu in the same M/R job then you would have to worry about library versioning conflicts otherwise.
I don't think it's possible or desirable to shade slf4j; see PARQUET-369 for an example of how that can go badly.
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] kudu-mapreduce: tweak poms
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: kudu-mapreduce: tweak poms
......................................................................
Patch Set 2:
oops disregard that.
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] kudu-mapreduce: tweak poms
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: kudu-mapreduce: tweak poms
......................................................................
Patch Set 2:
I forgot to document the ACLs.
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] kudu-mapreduce: tweak poms
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: kudu-mapreduce: tweak poms
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6418/1//COMMIT_MSG
Commit Message:
Line 22: fat jar containing kudu-client and kudu-mapreduce.
> Just passing through. For the tools, as long as they aren't intended to be
kudu-mapreduce only has three compile-scope dependencies:
* com.stumbleupon.async
* org.apache.kudu.kudu-client
* org.slf4j.slf4j-api
We aren't currently doing any shading, however I don't think these are really that important to shade (but I could probably be convinced otherwise).
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] kudu-mapreduce: tweak poms
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: kudu-mapreduce: tweak poms
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6418/1//COMMIT_MSG
Commit Message:
Line 22: fat jar containing kudu-client and kudu-mapreduce.
This means the kudu-client will also be shaded, right? We have client APIs as part of our MR APIs (the InputFormat gives you RowResult for example) so at least it would have to be provided.
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] kudu-mapreduce: tweak poms
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: kudu-mapreduce: tweak poms
......................................................................
kudu-mapreduce: tweak poms
This changes a few things about the mapreduce poms:
* kudu-mapreduce's dependency on hadoop-client is changed to provided.
* kudu-mapreduce no longer uses the maven assembly plugin. The
jar-with-dependencies artifact was not being published or used.
* kudu-client-tools now produces a fat jar, using the shade plugin. This
artifact is meant to be used as a binary MapReduce application, so
including all of the dependencies is appropriate.
Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Reviewed-on: http://gerrit.cloudera.org:8080/6418
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans <jd...@apache.org>
---
D java/assembly.xml
M java/kudu-client-tools/pom.xml
M java/kudu-mapreduce/pom.xml
3 files changed, 20 insertions(+), 62 deletions(-)
Approvals:
Jean-Daniel Cryans: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] kudu-mapreduce: tweak poms
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: kudu-mapreduce: tweak poms
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6418/1//COMMIT_MSG
Commit Message:
Line 22: fat jar containing kudu-client and kudu-mapreduce.
> kudu-client won't be shaded in kudu-mapreduce or kudu-client-tools. It is
Just passing through. For the tools, as long as they aren't intended to be used as libraries, shading doesn't matter. For the kudu-mapreduce fat jar, wouldn't we want all non-public APIs to be shaded? For example, Guava should still be shaded, right?
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] kudu-mapreduce: tweak poms
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: kudu-mapreduce: tweak poms
......................................................................
Patch Set 2:
async is part of the kudu-client public API. If we don't publicly expose kudu-client in the kudu-mapreduce API then we can shade both, if not I think it needs to remain unshaded.
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] kudu-mapreduce: tweak poms
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: kudu-mapreduce: tweak poms
......................................................................
Patch Set 2:
async's part of the public API which means we _cant_ shade it, right? if we shaded it it would be hard/impossible to code against, no?
FWIW this is why we marked the async API as unstable, iirc.
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] kudu-mapreduce: tweak poms
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6418
to look at the new patch set (#3).
Change subject: kudu-mapreduce: tweak poms
......................................................................
kudu-mapreduce: tweak poms
This changes a few things about the mapreduce poms:
* kudu-mapreduce's dependency on hadoop-client is changed to provided.
* kudu-mapreduce no longer uses the maven assembly plugin. The
jar-with-dependencies artifact was not being published or used.
* kudu-client-tools now produces a fat jar, using the shade plugin. This
artifact is meant to be used as a binary MapReduce application, so
including all of the dependencies is appropriate.
Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
---
D java/assembly.xml
M java/kudu-client-tools/pom.xml
M java/kudu-mapreduce/pom.xml
3 files changed, 20 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/6418/3
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] kudu-mapreduce: tweak poms
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: kudu-mapreduce: tweak poms
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6418/1//COMMIT_MSG
Commit Message:
Line 22: fat jar containing kudu-client and kudu-mapreduce.
> This means the kudu-client will also be shaded, right? We have client APIs
kudu-client won't be shaded in kudu-mapreduce or kudu-client-tools. It is bundled into the fat jar, but not shaded. Are you suggesting we shouldn't be bundling kudu-client into kudu-mapreduce? I think I might buy that. I do think it should be bundled into kudu-client-tools, since it's purpose is as an application binary, not a library.
--
To view, visit http://gerrit.cloudera.org:8080/6418
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I599ea9610fc3d541847b0205e959e94fb8425b2e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes