You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2016/12/02 16:39:02 UTC
[kudu-CR] flume sink: Fix jar shading
Hello Dan Burkert,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5327
to review the following change.
Change subject: flume sink: Fix jar shading
......................................................................
flume sink: Fix jar shading
Previously, when generating the kudu-flume-sink jar, we pulled in many
unshaded dependencies. This change improves the situation considerably
by doing the following:
1. Make the hadoop jars provided. This is find because Flume always puts
the Hadoop jars for the version it is running against on its
classpath.
2. Use the transitive shading provided by the kudu-client jar. This
makes it so that the kudu-client dependencies don't pollute the
classpath. Previously, unshaded kudu-client dependencies were being
included (which was surprising).
After this change the only non-org.apache.kudu classes in the classpath
are inherited from kudu-client, and once we fix KUDU-1780 then that will
be resolved as well.
Change-Id: I0ac2b67636c9f13f56cec2ea526d42c9e11f6b78
---
M java/kudu-flume-sink/pom.xml
1 file changed, 7 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/5327/1
--
To view, visit http://gerrit.cloudera.org:8080/5327
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0ac2b67636c9f13f56cec2ea526d42c9e11f6b78
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] flume sink: Fix jar shading
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has uploaded a new patch set (#2).
Change subject: flume sink: Fix jar shading
......................................................................
flume sink: Fix jar shading
Previously, when generating the kudu-flume-sink jar, we pulled in many
unshaded dependencies. This change improves the situation considerably
by doing the following:
1. Make the hadoop jars provided. This is fine because Flume always puts
the Hadoop jars for the version it is running against on its
classpath.
2. Use the transitive shading provided by the kudu-client jar. This
makes it so that the kudu-client dependencies don't pollute the
classpath. Previously, unshaded kudu-client dependencies were being
included in the kudu-flume-sink jar (which I found surprising).
After this change the only non-org.apache.kudu classes in the classpath
are inherited from kudu-client, and once we fix KUDU-1780 then that will
be resolved as well.
Change-Id: I0ac2b67636c9f13f56cec2ea526d42c9e11f6b78
---
M java/kudu-flume-sink/pom.xml
1 file changed, 7 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/5327/2
--
To view, visit http://gerrit.cloudera.org:8080/5327
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0ac2b67636c9f13f56cec2ea526d42c9e11f6b78
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] flume sink: Fix jar shading
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: flume sink: Fix jar shading
......................................................................
Patch Set 2:
> Could a test check for the expected shading? Would a test like that
> fit into the current infrastructure?
That's a good idea, and I think we should do it, but as a follow-up patch.
You could run a script that does something like "jar tf foo.jar" and then greps for anything not starting with org/apache/kudu and throws an error if it finds something not in a whitelist.
It could be triggered as part of build-support/jenkins/build-and-test.sh
--
To view, visit http://gerrit.cloudera.org:8080/5327
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac2b67636c9f13f56cec2ea526d42c9e11f6b78
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] flume sink: Fix jar shading
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: flume sink: Fix jar shading
......................................................................
flume sink: Fix jar shading
Previously, when generating the kudu-flume-sink jar, we pulled in many
unshaded dependencies. This change improves the situation considerably
by doing the following:
1. Make the hadoop jars provided. This is fine because Flume always puts
the Hadoop jars for the version it is running against on its
classpath.
2. Use the transitive shading provided by the kudu-client jar. This
makes it so that the kudu-client dependencies don't pollute the
classpath. Previously, unshaded kudu-client dependencies were being
included in the kudu-flume-sink jar (which I found surprising).
After this change the only non-org.apache.kudu classes in the classpath
are inherited from kudu-client, and once we fix KUDU-1780 then that will
be resolved as well.
Change-Id: I0ac2b67636c9f13f56cec2ea526d42c9e11f6b78
Reviewed-on: http://gerrit.cloudera.org:8080/5327
Reviewed-by: Will Berkeley <wd...@gmail.com>
Tested-by: Kudu Jenkins
---
M java/kudu-flume-sink/pom.xml
1 file changed, 7 insertions(+), 24 deletions(-)
Approvals:
Will Berkeley: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5327
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I0ac2b67636c9f13f56cec2ea526d42c9e11f6b78
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] flume sink: Fix jar shading
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.
Change subject: flume sink: Fix jar shading
......................................................................
Patch Set 2: Code-Review+2
LGTM provisional on build passing.
Could a test check for the expected shading? Would a test like that fit into the current infrastructure?
--
To view, visit http://gerrit.cloudera.org:8080/5327
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac2b67636c9f13f56cec2ea526d42c9e11f6b78
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] flume sink: Fix jar shading
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: flume sink: Fix jar shading
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5327/1/java/kudu-flume-sink/pom.xml
File java/kudu-flume-sink/pom.xml:
Line 29: <build>
While you are modifying this, could you move this <build> block below the dependencies? That's how all our other poms have it.
Line 38: <!-- We want to transitively shade what kudu-client shades. -->
I don't think this comment is right - this shades the kudu client classes, kudu-flume shouldn't need to know or worry that internally kudu-client uses shaded classes.
Line 39: <include>org.apache.kudu:kudu-client</include>
I think this can be simplified to
<include>*:*</include>
that's what kudu-spark does, so it's probably best to be consistent (unless there's something I'm missing and that won't work for this case).
--
To view, visit http://gerrit.cloudera.org:8080/5327
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac2b67636c9f13f56cec2ea526d42c9e11f6b78
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] flume sink: Fix jar shading
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: flume sink: Fix jar shading
......................................................................
Patch Set 1:
woops - sorry I didn't realize this was merged already. None of my comments are critical
--
To view, visit http://gerrit.cloudera.org:8080/5327
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac2b67636c9f13f56cec2ea526d42c9e11f6b78
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] flume sink: Fix jar shading
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: flume sink: Fix jar shading
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5327/1/java/kudu-flume-sink/pom.xml
File java/kudu-flume-sink/pom.xml:
Line 38: <!-- We want to transitively shade what kudu-client shades. -->
> I don't think this comment is right - this shades the kudu client classes,
If I remove this, then all of the kudu-client deps get included in the jar without any shading. I'm not sure why.
Line 39: <include>org.apache.kudu:kudu-client</include>
> I think this can be simplified to
interesting.
--
To view, visit http://gerrit.cloudera.org:8080/5327
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac2b67636c9f13f56cec2ea526d42c9e11f6b78
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes