Committed r1575343 to branch-4.2

ok fine, I just added a note as a reminder.

I would like we resolve BOOKKEEPER-730 before merging this to 4.2.3. Rakesh R

Adding a note:
This is not merged to 4.2.3 branch but in fix version its mentioned as 4.2.3. Since we raised a separate blocker BOOKKEEPER-730 to discuss the licensing issue, I feel we can merge this to 4.2.3 branch as well.

SUCCESS: Integrated in bookkeeper-trunk #533 (See https://builds.apache.org/job/bookkeeper-trunk/533/)
BOOKKEEPER-708: Shade protobuf library to avoid incompatible versions (rakesh, ivank via sijie) (sijie: rev 1563566)

• /zookeeper/bookkeeper/trunk/CHANGES.txt
• /zookeeper/bookkeeper/trunk/bookkeeper-server/pom.xml
• /zookeeper/bookkeeper/trunk/bookkeeper-stats-providers/codahale-metrics-provider/pom.xml
• /zookeeper/bookkeeper/trunk/hedwig-client/pom.xml
• /zookeeper/bookkeeper/trunk/hedwig-protocol/pom.xml
• /zookeeper/bookkeeper/trunk/pom.xml

+1 for the latest patch.

resolved as r1563566.

lgtm, +1. Sijie Guo?

Testing JIRA BOOKKEEPER-708

Patch 0003-BOOKKEEPER-708-trunk-shade-protobuf-guava-with-minjar.patch downloaded at Tue Jan 28 10:46:40 UTC 2014

----------------------------

+1 PATCH_APPLIES
+1 CLEAN
-1 RAW_PATCH_ANALYSIS
. +1 the patch does not introduce any @author tags
. +1 the patch does not introduce any tabs
. +1 the patch does not introduce any trailing spaces
. +1 the patch does not introduce any line longer than 120
. -1 the patch does not add/modify any testcase
+1 RAT
. +1 the patch does not seem to introduce new RAT warnings
+1 JAVADOC
. +1 the patch does not seem to introduce new Javadoc warnings
+1 COMPILE
. +1 HEAD compiles
. +1 patch compiles
. +1 the patch does not seem to introduce new javac warnings
+1 FINDBUGS
. +1 the patch does not seem to introduce new Findbugs warnings
+1 TESTS
. Tests run: 893
+1 DISTRO
. +1 distro tarball builds with the patch

----------------------------
-1 Overall result, please check the reported -1(s)

The full output of the test-patch run is available at

. https://builds.apache.org/job/bookkeeper-trunk-precommit-build/568/

Updated latest patch which addresses the comments.

wrong indent in the pom file. and I don't think you are putting the guava dependency for hedwig in right place, since 'hedwig-protocol' doesn't depend on guava. it should be 'hedwig-client'.

New patch looks good to me. +1

Testing JIRA BOOKKEEPER-708

Patch 0002-BOOKKEEPER-708-trunk-shade-protobuf-guava-with-minjar.patch downloaded at Mon Jan 27 12:11:37 UTC 2014

----------------------------

+1 PATCH_APPLIES
+1 CLEAN
-1 RAW_PATCH_ANALYSIS
. +1 the patch does not introduce any @author tags
. +1 the patch does not introduce any tabs
. +1 the patch does not introduce any trailing spaces
. +1 the patch does not introduce any line longer than 120
. -1 the patch does not add/modify any testcase
+1 RAT
. +1 the patch does not seem to introduce new RAT warnings
+1 JAVADOC
. +1 the patch does not seem to introduce new Javadoc warnings
+1 COMPILE
. +1 HEAD compiles
. +1 patch compiles
. +1 the patch does not seem to introduce new javac warnings
+1 FINDBUGS
. +1 the patch does not seem to introduce new Findbugs warnings
+1 TESTS
. Tests run: 893
+1 DISTRO
. +1 distro tarball builds with the patch

----------------------------
-1 Overall result, please check the reported -1(s)

The full output of the test-patch run is available at

. https://builds.apache.org/job/bookkeeper-trunk-precommit-build/567/

Thanks Ivan for the minimize jar option. Attached patch addressing Sijie's comment, where I have shaded 'guava' and 'protobuf' only in the bkserver side and hedwig components are still continuing with non-shaded items.

Testing JIRA BOOKKEEPER-708

Patch 0001-BOOKKEEPER-708-Shade-libraries-to-avoid-incompatible.patch downloaded at Sat Jan 25 05:42:16 UTC 2014

----------------------------

+1 PATCH_APPLIES
+1 CLEAN
-1 RAW_PATCH_ANALYSIS
. +1 the patch does not introduce any @author tags
. +1 the patch does not introduce any tabs
. +1 the patch does not introduce any trailing spaces
. +1 the patch does not introduce any line longer than 120
. -1 the patch does not add/modify any testcase
+1 RAT
. +1 the patch does not seem to introduce new RAT warnings
+1 JAVADOC
. +1 the patch does not seem to introduce new Javadoc warnings
-1 COMPILE
. +1 HEAD compiles
. -1 patch does not compile
. +1 the patch does not seem to introduce new javac warnings
+1 FINDBUGS
. +1 the patch does not seem to introduce new Findbugs warnings
-1 TESTS - patch does not compile, cannot run testcases
-1 DISTRO
. -1 distro tarball fails with the patch

----------------------------
-1 Overall result, please check the reported -1(s)

The full output of the test-patch run is available at

. https://builds.apache.org/job/bookkeeper-trunk-precommit-build/565/

BTW, wrong indent on bookkeeper-server/pom.xml

+                <artifactSet>
+                  <includes>
+                    <include>com.google.protobuf:protobuf-java</include>
+                   <include>com.google.guava:guava</include>
+                   <include>org.jboss.netty:netty</include>
+                  </includes>
+                </artifactSet>

-1 for shading netty, since ClientSocketChannelFactory is used as constructor for BookKeeper. If we shade netty, we can't share same ClientSocketChannelFactory with bookkeeper client.

as I said before, don't shade libraries that used in public method. and if you want to shade netty, please clarify what's the issue before shading.

good for me, +1.

minimizeJar is the option to make the jar smaller by only pulling in whats used.

New patch. Shades guava, protobuf and netty, as these are likely to be present in other applications. jar size is 2.5M.

Just so that I understand, we are introducing the ability of producing a reduced jar vs. an uber jar for bookkeeper-server. How do we actually control it? Is it by changing the pom.xml:

<createDependencyReducedPom>true</createDependencyReducedPom>

I actually tried it and it didn't seem to make much difference.

+1 from the latest patch. If I got +1 from Flavio Junqueira, will push it later.

Thanks Ivan for the clarification. If everyone agrees, can we push this in?

I don't see any need to upgrade. Upgrading would involve installing 2.5.0 on my machine, and it's a pain to have multiple installs of protoc. Re: protobuf wireformat BC, once we shade we shouldn't even have to worry until we upgrade protobuf version.

Hi Flavio/Sijie,
I couldn't see any version mismatch with respect to this patch/fix, we are still continuing with 2.4.1 version. Anything am missing?

Flavio Junqueira I think protobuf handles format compatibility between versions. so it might be OK. but that's a valid point, we should double-check on this, to see if it works correctly when a client use protobuf 2.4 reading metadata written by higher protobuf.

What Ivan Kelly says about client vs. server seems right to me, although someone running a client will most likely be running a server as well. I don't see a reason for that being a problem, but can anyone think of a problem in the case the client and server are running different versions of netty? is there a concern of any form if that's the case?

Testing JIRA BOOKKEEPER-708

Patch 0001-BOOKKEEPER-708-Shade-protobuf-library-to-avoid-incom.patch downloaded at Wed Jan 15 14:00:48 UTC 2014

----------------------------

+1 PATCH_APPLIES
+1 CLEAN
-1 RAW_PATCH_ANALYSIS
. +1 the patch does not introduce any @author tags
. +1 the patch does not introduce any tabs
. +1 the patch does not introduce any trailing spaces
. +1 the patch does not introduce any line longer than 120
. -1 the patch does not add/modify any testcase
+1 RAT
. +1 the patch does not seem to introduce new RAT warnings
+1 JAVADOC
. +1 the patch does not seem to introduce new Javadoc warnings
+1 COMPILE
. +1 HEAD compiles
. +1 patch compiles
. +1 the patch does not seem to introduce new javac warnings
+1 FINDBUGS
. +1 the patch does not seem to introduce new Findbugs warnings
+1 TESTS
. Tests run: 885
+1 DISTRO
. +1 distro tarball builds with the patch

----------------------------
-1 Overall result, please check the reported -1(s)

The full output of the test-patch run is available at

. https://builds.apache.org/job/bookkeeper-trunk-precommit-build/556/

I've set createDependencyReducedPom to true in a new patch. Regarding the jar in the dist bin package, I don't think it's easy to exclude it without hacking around it, any having it there does no harm. Shading the deps is only important for the client. The dist bin package is the server.

it doesn't really matter whether the jar is in the distributed package or not, as it is already a binary package. but it matters in pom file as it is the source of dependencies.

One more thing is, once we shade the protobuf jar, then we can exclude the same jar in binary distribution package.

I think we need to set 'createDependencyReducedPom' to true. Otherwise, it would still pull the protobuf version in, as the pom file will not be changed.

BTW, Vinay's suggestion is good. we might need to keep shade plugins use same version.

all other pom.xml, maven shade plugin used is of version 1.5.
Do upgrade to 2.2 needed in all places..?

+1 looks good.

Ivan Kelly let's shade guava when we really need do that. if you are ok with that, I will commit that.

As HDFS-5518 points out, simply mandating that people use a newer guava is not without risks. I still think we should shade guava, but do so with the reduced jar option to avoid polluting the namespace as Sijie Guo says.

Thanks Sijie Guo for your comments. Attached latest patch, here I just shaded only the protobufs.

Testing JIRA BOOKKEEPER-708

Patch 0002-BOOKKEEPER-708.patch downloaded at Thu Nov 14 14:18:49 UTC 2013

----------------------------

+1 PATCH_APPLIES
+1 CLEAN
-1 RAW_PATCH_ANALYSIS
. +1 the patch does not introduce any @author tags
. +1 the patch does not introduce any tabs
. +1 the patch does not introduce any trailing spaces
. +1 the patch does not introduce any line longer than 120
. -1 the patch does not add/modify any testcase
+1 RAT
. +1 the patch does not seem to introduce new RAT warnings
+1 JAVADOC
. +1 the patch does not seem to introduce new Javadoc warnings
+1 COMPILE
. +1 HEAD compiles
. +1 patch compiles
. +1 the patch does not seem to introduce new javac warnings
+1 FINDBUGS
. +1 the patch does not seem to introduce new Findbugs warnings
+1 TESTS
. Tests run: 883
+1 DISTRO
. +1 distro tarball builds with the patch

----------------------------
-1 Overall result, please check the reported -1(s)

The full output of the test-patch run is available at

. https://builds.apache.org/job/bookkeeper-trunk-precommit-build/539/

Thanks a lot Steve Loughran for the interest. Also, nice to hear the upgradation of guava version. Guava's latest release is 15.0 version, first will try running HDFS with guava 15.0 version and will udpdate the patch in HADOOP-9991.

I you can provide a patch to update HDFS and link it to HADOOP-9991 I'll give it a look. Shading hides a problem that can still surface later -and we don't have an particular reason to stick to guava 11.0.2 that I'm aware of

when you bump bookkeeper version in BKJM, why can't you bump HDFS's guava version ? it is a more straightforward thing, as BKJM is dependent on bookkeeper, not bookkeeper dependent on BKJM. avoiding unnecessary shade will produce a clean namespace in bookkeeper, which is easy for maintenance.

Sijie Guo I haven't observed any compatibility issues across the versions. But I'm doubting, HDFS community will upgrade the version from 11.0.2 to 13.0.1 for a pluggable module. Whats your opinion?

BTW Bookkeeper requires guava 13.0.1 version as we are using new features like 'com.google.common.util.concurrent.RateLimiter' which doesn't exists in older version.

Rakesh R could you try not shade guava? if there is a BC issue, point them out.

For safer side, I've shaded guava also. Since we are using higher guava version, it shoudn't break HDFS becuase of our feature inclusion.

Hi,
While regression testing, have seen HDFS is using older version of guava(11.0.2) and this also included in shade.
Attached patch which shaded protobuf, guava. Also, I have considered only for the bookkeeper-server.