[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 Gerrit-Reviewer: Jean-Daniel Cryans
[kudu-CR] kudu-mapreduce: tweak poms
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 Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] kudu-mapreduce: tweak poms
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] kudu-mapreduce: tweak poms
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] kudu-mapreduce: tweak poms
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] kudu-mapreduce: tweak poms
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] kudu-mapreduce: tweak poms
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] kudu-mapreduce: tweak poms
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] kudu-mapreduce: tweak poms
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] kudu-mapreduce: tweak poms
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] kudu-mapreduce: tweak poms
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] kudu-mapreduce: tweak poms
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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] kudu-mapreduce: tweak poms
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 --- 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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon