Hadoop Common
  1. Hadoop Common
  2. HADOOP-7379

Add ability to include Protobufs in ObjectWritable

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: 0.23.0
    • Component/s: io, ipc
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Protocol buffer-generated types may now be used as arguments or return values for Hadoop RPC.

      Description

      Per HDFS-2060, it would make it easier to piecemeal switch to protocol buffer based data structures in the wire protocol if we could intermix the two. The IPC framework currently provides the concept of "engines" for RPC, but that doesn't easily allow mixed types within the same framework for ease of transition.

      I'd like to add the cases to ObjectWritable to be handle subclasses of Message, the superclass of codegenned protobufs.

      1. hadoop-7379.txt
        17 kB
        Todd Lipcon
      2. hadoop-7379.txt
        14 kB
        Todd Lipcon

        Issue Links

          Activity

          Todd Lipcon created issue -
          Todd Lipcon made changes -
          Field Original Value New Value
          Link This issue blocks HDFS-2060 [ HDFS-2060 ]
          Hide
          Todd Lipcon added a comment -

          Here's a patch which does the following:

          • protobufs can be serialized by ObjectWritable. They are length-prefixed with a varint and then written on the wire
          • adds test for the writable format itself
          • adds a call in TestIPC that uses a protobuf as a parameter and response

          I see one spot for an optimization by removing a buffer copy on the read side, but I'd like to leave that for later.

          Show
          Todd Lipcon added a comment - Here's a patch which does the following: protobufs can be serialized by ObjectWritable. They are length-prefixed with a varint and then written on the wire adds test for the writable format itself adds a call in TestIPC that uses a protobuf as a parameter and response I see one spot for an optimization by removing a buffer copy on the read side, but I'd like to leave that for later.
          Todd Lipcon made changes -
          Attachment hadoop-7379.txt [ 12482124 ]
          Todd Lipcon made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12482124/hadoop-7379.txt
          against trunk revision 1134218.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/612//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/612//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/612//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12482124/hadoop-7379.txt against trunk revision 1134218. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/612//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/612//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/612//console This message is automatically generated.
          Hide
          Luke Lu added a comment -

          Todd, what PEDs are you taking? I mean, you're on a roll

          ProtoUtil.readRawVarint32 is a new implementation vint32 decoding for DataInput. It needs test coverage for 1 to 5 bytes.

          Show
          Luke Lu added a comment - Todd, what PEDs are you taking? I mean, you're on a roll ProtoUtil.readRawVarint32 is a new implementation vint32 decoding for DataInput. It needs test coverage for 1 to 5 bytes.
          Hide
          Todd Lipcon added a comment -

          Todd, what PEDs are you taking? I mean, you're on a roll

          I recently learned how to make pretty good cappuccinos from our machine here at the office. No joke, I'm considering a part time job at Blue Bottle. Thanks

          ProtoUtil.readRawVarint32 is a new implementation vint32 decoding for DataInput. It needs test coverage for 1 to 5 bytes.

          I actually copy-pasted it from protobufs (compatible license). I can probably copy-paste some of their unit tests, too.. I'll look around.

          Show
          Todd Lipcon added a comment - Todd, what PEDs are you taking? I mean, you're on a roll I recently learned how to make pretty good cappuccinos from our machine here at the office. No joke, I'm considering a part time job at Blue Bottle. Thanks ProtoUtil.readRawVarint32 is a new implementation vint32 decoding for DataInput. It needs test coverage for 1 to 5 bytes. I actually copy-pasted it from protobufs (compatible license). I can probably copy-paste some of their unit tests, too.. I'll look around.
          Hide
          Todd Lipcon added a comment -

          Added a simple unit test that encodes ints with CodedOutputStream and then decodes them with the function in ProtoUtil

          Show
          Todd Lipcon added a comment - Added a simple unit test that encodes ints with CodedOutputStream and then decodes them with the function in ProtoUtil
          Todd Lipcon made changes -
          Attachment hadoop-7379.txt [ 12482130 ]
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12482130/hadoop-7379.txt
          against trunk revision 1134218.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 7 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/613//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/613//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/613//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12482130/hadoop-7379.txt against trunk revision 1134218. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/613//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/613//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/613//console This message is automatically generated.
          Hide
          Arun C Murthy added a comment -

          Woah, tlipcon, slow down man!

          In all seriousness... great stuff.

          What will it take to get ipc.Server to understand PB rather than Writable?

          Show
          Arun C Murthy added a comment - Woah, tlipcon, slow down man! In all seriousness... great stuff. What will it take to get ipc.Server to understand PB rather than Writable?
          Hide
          Todd Lipcon added a comment -

          What will it take to get ipc.Server to understand PB rather than Writable?

          You mean using PBs for the actual invocations? Or the "header" type info at the beginning? I think most of the value from PB comes from the evolvable parameter types, but moving to PB for the headers would also allow us to do things like add extra metadata to requests without breaking previous versions (eg dapper-style tracing info?)

          Show
          Todd Lipcon added a comment - What will it take to get ipc.Server to understand PB rather than Writable? You mean using PBs for the actual invocations? Or the "header" type info at the beginning? I think most of the value from PB comes from the evolvable parameter types, but moving to PB for the headers would also allow us to do things like add extra metadata to requests without breaking previous versions (eg dapper-style tracing info?)
          Hide
          Todd Lipcon added a comment -

          What do you guys think, is this a reasonable path?

          Show
          Todd Lipcon added a comment - What do you guys think, is this a reasonable path?
          Hide
          Luke Lu added a comment -

          +1, lgtm.

          Show
          Luke Lu added a comment - +1, lgtm.
          Hide
          Todd Lipcon added a comment -

          Committed to trunk. Thanks for reviewing, Luke!

          I haven't marked this as an incompatible change, since it simply adds a new feature to ObjectWritable without changing encoding of any existing features.

          Show
          Todd Lipcon added a comment - Committed to trunk. Thanks for reviewing, Luke! I haven't marked this as an incompatible change, since it simply adds a new feature to ObjectWritable without changing encoding of any existing features.
          Todd Lipcon made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Release Note Protocol buffer-generated types may now be used as arguments or return values for Hadoop RPC.
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #657 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/657/)
          HADOOP-7379. Add the ability to serialize and deserialize protocol buffers in ObjectWritable. Contributed by Todd Lipcon.

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1136222
          Files :

          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/DataOutputOutputStream.java
          • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/ipc/TestRPC.java
          • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/util/TestProtoUtil.java
          • /hadoop/common/trunk/common/CHANGES.txt
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/util/ProtoUtil.java
          • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/TestObjectWritableProtos.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/ObjectWritable.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #657 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/657/ ) HADOOP-7379 . Add the ability to serialize and deserialize protocol buffers in ObjectWritable. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1136222 Files : /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/DataOutputOutputStream.java /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/ipc/TestRPC.java /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/util/TestProtoUtil.java /hadoop/common/trunk/common/CHANGES.txt /hadoop/common/trunk/common/src/java/org/apache/hadoop/util/ProtoUtil.java /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/TestObjectWritableProtos.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/ObjectWritable.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-maven #43 (See https://builds.apache.org/job/Hadoop-Common-trunk-maven/43/)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-maven #43 (See https://builds.apache.org/job/Hadoop-Common-trunk-maven/43/ )
          Arun C Murthy made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Todd Lipcon
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development