Uploaded image for project: 'Bookkeeper'
  1. Bookkeeper
  2. BOOKKEEPER-708

Shade protobuf library to avoid incompatible versions

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.3.0, 4.2.3
    • Component/s: bookkeeper-server
    • Labels:
      None

      Description

      as offline discussion, we need to shade protobuf library for BKJM as hadoop uses protobuf 2.5.

      this is planned on version 4.2.3 and 4.3.0.

        Issue Links

          Activity

          Hide
          ikelly Ivan Kelly added a comment -

          Committed r1575343 to branch-4.2

          Show
          ikelly Ivan Kelly added a comment - Committed r1575343 to branch-4.2
          Hide
          rakeshr Rakesh R added a comment -

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

          Show
          rakeshr Rakesh R added a comment - ok fine, I just added a note as a reminder.
          Hide
          hustlmsp Sijie Guo added a comment -

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

          Show
          hustlmsp Sijie Guo added a comment - I would like we resolve BOOKKEEPER-730 before merging this to 4.2.3. Rakesh R
          Hide
          rakeshr Rakesh R added a comment -

          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.

          Show
          rakeshr Rakesh R added a comment - 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.
          Hide
          hudson Hudson added a comment -

          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
          Show
          hudson Hudson added a comment - 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
          Hide
          hustlmsp Sijie Guo added a comment -

          +1 for the latest patch.

          resolved as r1563566.

          Show
          hustlmsp Sijie Guo added a comment - +1 for the latest patch. resolved as r1563566.
          Hide
          fpj Flavio Junqueira added a comment -

          lgtm, +1. Sijie Guo?

          Show
          fpj Flavio Junqueira added a comment - lgtm, +1. Sijie Guo ?
          Hide
          hadoopqa Hadoop QA added a comment -

          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/

          Show
          hadoopqa Hadoop QA added a comment - 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/
          Hide
          rakeshr Rakesh R added a comment -

          Updated latest patch which addresses the comments.

          Show
          rakeshr Rakesh R added a comment - Updated latest patch which addresses the comments.
          Hide
          hustlmsp Sijie Guo added a comment -

          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'.

          Show
          hustlmsp Sijie Guo added a comment - 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'.
          Hide
          ikelly Ivan Kelly added a comment -

          New patch looks good to me. +1

          Show
          ikelly Ivan Kelly added a comment - New patch looks good to me. +1
          Hide
          hadoopqa Hadoop QA added a comment -

          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/

          Show
          hadoopqa Hadoop QA added a comment - 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/
          Hide
          rakeshr Rakesh R added a comment -

          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.

          Show
          rakeshr Rakesh R added a comment - 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.
          Hide
          hadoopqa Hadoop QA added a comment -

          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/

          Show
          hadoopqa Hadoop QA added a comment - 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/
          Hide
          hustlmsp Sijie Guo added a comment -

          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>
          
          Show
          hustlmsp Sijie Guo added a comment - 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>
          Hide
          hustlmsp Sijie Guo added a comment -

          -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.

          Show
          hustlmsp Sijie Guo added a comment - -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.
          Hide
          fpj Flavio Junqueira added a comment -

          good for me, +1.

          Show
          fpj Flavio Junqueira added a comment - good for me, +1.
          Hide
          ikelly Ivan Kelly added a comment -

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

          Show
          ikelly Ivan Kelly added a comment - minimizeJar is the option to make the jar smaller by only pulling in whats used.
          Hide
          ikelly Ivan Kelly added a comment -

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

          Show
          ikelly Ivan Kelly added a comment - New patch. Shades guava, protobuf and netty, as these are likely to be present in other applications. jar size is 2.5M.
          Hide
          fpj Flavio Junqueira added a comment -

          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.

          Show
          fpj Flavio Junqueira added a comment - 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.
          Hide
          hustlmsp Sijie Guo added a comment -

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

          Show
          hustlmsp Sijie Guo added a comment - +1 from the latest patch. If I got +1 from Flavio Junqueira , will push it later.
          Hide
          rakeshr Rakesh R added a comment -

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

          Show
          rakeshr Rakesh R added a comment - Thanks Ivan for the clarification. If everyone agrees, can we push this in?
          Hide
          ikelly Ivan Kelly added a comment -

          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.

          Show
          ikelly Ivan Kelly added a comment - 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.
          Hide
          rakeshr Rakesh R added a comment -

          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?

          Show
          rakeshr Rakesh R added a comment - 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?
          Hide
          hustlmsp Sijie Guo added a comment -

          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.

          Show
          hustlmsp Sijie Guo added a comment - 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.
          Hide
          fpj Flavio Junqueira added a comment -

          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?

          Show
          fpj Flavio Junqueira added a comment - 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?
          Hide
          hadoopqa Hadoop QA added a comment -

          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/

          Show
          hadoopqa Hadoop QA added a comment - 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/
          Hide
          ikelly Ivan Kelly added a comment -

          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.

          Show
          ikelly Ivan Kelly added a comment - 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.
          Hide
          hustlmsp Sijie Guo added a comment -

          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.

          Show
          hustlmsp Sijie Guo added a comment - 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.
          Hide
          vinayrpet Vinayakumar B added a comment -

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

          Show
          vinayrpet Vinayakumar B added a comment - One more thing is, once we shade the protobuf jar, then we can exclude the same jar in binary distribution package.
          Hide
          hustlmsp Sijie Guo added a comment -

          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.

          Show
          hustlmsp Sijie Guo added a comment - 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.
          Hide
          vinayrpet Vinayakumar B added a comment -

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

          Show
          vinayrpet Vinayakumar B added a comment - all other pom.xml, maven shade plugin used is of version 1.5. Do upgrade to 2.2 needed in all places..?
          Hide
          hustlmsp Sijie Guo added a comment -

          +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.

          Show
          hustlmsp Sijie Guo added a comment - +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.
          Hide
          ikelly Ivan Kelly added a comment -

          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.

          Show
          ikelly Ivan Kelly added a comment - 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.
          Hide
          rakeshr Rakesh R added a comment -

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

          Show
          rakeshr Rakesh R added a comment - Thanks Sijie Guo for your comments. Attached latest patch, here I just shaded only the protobufs.
          Hide
          hadoopqa Hadoop QA added a comment -

          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/

          Show
          hadoopqa Hadoop QA added a comment - 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/
          Hide
          rakeshr Rakesh R added a comment -

          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.

          Show
          rakeshr Rakesh R added a comment - 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 .
          Hide
          stevel@apache.org Steve Loughran added a comment -

          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

          Show
          stevel@apache.org Steve Loughran added a comment - 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
          Hide
          hustlmsp Sijie Guo added a comment -

          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.

          Show
          hustlmsp Sijie Guo added a comment - 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.
          Hide
          rakeshr Rakesh R added a comment -

          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.

          Show
          rakeshr Rakesh R added a comment - 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.
          Hide
          hustlmsp Sijie Guo added a comment -

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

          Show
          hustlmsp Sijie Guo added a comment - Rakesh R could you try not shade guava? if there is a BC issue, point them out.
          Hide
          rakeshr Rakesh R added a comment -

          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.

          Show
          rakeshr Rakesh R added a comment - 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.
          Hide
          rakeshr Rakesh R added a comment -

          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.

          Show
          rakeshr Rakesh R added a comment - 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.

            People

            • Assignee:
              rakeshr Rakesh R
              Reporter:
              hustlmsp Sijie Guo
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development