Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Release Note:
      RPC can use Avro serialization.

      Description

      Permit RPC protocols to use Avro to serialize requests and responses, so that protocols may better evolve without breaking compatibility.

      1. HADOOP-6170.patch
        17 kB
        Doug Cutting
      2. HADOOP-6170.patch
        16 kB
        Doug Cutting
      3. HADOOP-6170.patch
        13 kB
        Doug Cutting
      4. HADOOP-6170.patch
        13 kB
        Doug Cutting
      5. HADOOP-6170.patch
        13 kB
        Doug Cutting

        Issue Links

          Activity

          Hide
          Doug Cutting added a comment -

          To implement this, an Avro Transciever can be implemented using Hadoop's Client and Server classes. The request and response Writable would contain just List<ByteBuffer>.

          Show
          Doug Cutting added a comment - To implement this, an Avro Transciever can be implemented using Hadoop's Client and Server classes. The request and response Writable would contain just List<ByteBuffer>.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12417826/HADOOP-6170.patch
          against trunk revision 807753.

          +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 generated 119 javac compiler warnings (more than the trunk's current 116 warnings).

          -1 findbugs. The patch appears to introduce 1 new Findbugs 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 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/628/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/628/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/628/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/628/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/12417826/HADOOP-6170.patch against trunk revision 807753. +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 generated 119 javac compiler warnings (more than the trunk's current 116 warnings). -1 findbugs. The patch appears to introduce 1 new Findbugs 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 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/628/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/628/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/628/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/628/console This message is automatically generated.
          Hide
          Doug Cutting added a comment -

          New patch that fixes lint warnings.

          Show
          Doug Cutting added a comment - New patch that fixes lint warnings.
          Hide
          Doug Cutting added a comment -

          Ignore the warning rather than add bogus code to make it go away.

          Show
          Doug Cutting added a comment - Ignore the warning rather than add bogus code to make it go away.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12417892/HADOOP-6170.patch
          against trunk revision 808415.

          +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 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 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/630/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/630/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/630/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/630/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/12417892/HADOOP-6170.patch against trunk revision 808415. +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 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 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/630/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/630/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/630/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/630/console This message is automatically generated.
          Hide
          Doug Cutting added a comment -

          Can another committer please review this? Thanks!

          Show
          Doug Cutting added a comment - Can another committer please review this? Thanks!
          Hide
          Philip Zeyliger added a comment -

          I took a look. Overall, looks good. I'm sufficiently behind on my Avro-fu that I
          had a hard time following some of the magic.

          I'm writing down below what my current understanding is. Let me know
          where I'm totally wrong. Do add some JavaDoc per class.

          TunnelProtocol is the meta-interface that's used by Hadoop IPC.

          BufferListWritable is used by Hadoop IPC to ship
          the Avro byte-stream around. Its serialization format is:

          data := size (buffer)[size]
          buffer := len (bytes)[len]
          size, len are writable integers
          bytes are raw bytes
          x[count] indicates count repetitions of x.
          

          ClientTranceiver delegates shipping of bytes via Hadoop IPC.
          ServerTranceiver doesn't do much of anything except store
          the response data. New instances of it are created at every
          call in TunnelResponder.

          TunnelResponder implements the Hadoop IPC (TunnelProtocol);
          it's the thing running on the server, and converts calls
          into Avro RPC calls. This class would be clearer if, instead
          of extending ReflectResponder, you kept a private ReflectResponder,
          and called that explicitly in call. Then you would rename
          TunnelResponder to TunnelProtocolImpl.

          Can a single RPC server satisfy multiple protocols? Does that
          work with AvroRPC? I don't think it can right now, but I think
          that's necessary, since several daemons implement a handful of
          protocols.

          Some specific notes:

          versioning features to for inter-Java RPCs.

          Typo.

          AvroTestProtocol is generated code, yes? Or does ReflectData.getProtocol()
          figure it out from reflection and paranamer data? If the former,
          should the source schema be checked in? Should the generation
          be done in build.xml?

          assertEquals(intResult, 3);

          Total nit: JUnit prefers the expected argument on the left.

          public BufferListWritable() {}

          Eclipse tells me this is unused. Might be worth
          a quick comment indicating that it's required because
          Writables get instantiated via reflection.

          getRemoteName() { return "remote"; }

          Should these be customized to the Protocol being used?

          Cheers,

          – Philip

          Show
          Philip Zeyliger added a comment - I took a look. Overall, looks good. I'm sufficiently behind on my Avro-fu that I had a hard time following some of the magic. I'm writing down below what my current understanding is. Let me know where I'm totally wrong. Do add some JavaDoc per class. TunnelProtocol is the meta-interface that's used by Hadoop IPC. BufferListWritable is used by Hadoop IPC to ship the Avro byte-stream around. Its serialization format is: data := size (buffer)[size] buffer := len (bytes)[len] size, len are writable integers bytes are raw bytes x[count] indicates count repetitions of x. ClientTranceiver delegates shipping of bytes via Hadoop IPC. ServerTranceiver doesn't do much of anything except store the response data. New instances of it are created at every call in TunnelResponder. TunnelResponder implements the Hadoop IPC (TunnelProtocol); it's the thing running on the server, and converts calls into Avro RPC calls. This class would be clearer if, instead of extending ReflectResponder, you kept a private ReflectResponder, and called that explicitly in call. Then you would rename TunnelResponder to TunnelProtocolImpl. Can a single RPC server satisfy multiple protocols? Does that work with AvroRPC? I don't think it can right now, but I think that's necessary, since several daemons implement a handful of protocols. Some specific notes: versioning features to for inter-Java RPCs. Typo. AvroTestProtocol is generated code, yes? Or does ReflectData.getProtocol() figure it out from reflection and paranamer data? If the former, should the source schema be checked in? Should the generation be done in build.xml? assertEquals(intResult, 3); Total nit: JUnit prefers the expected argument on the left. public BufferListWritable() {} Eclipse tells me this is unused. Might be worth a quick comment indicating that it's required because Writables get instantiated via reflection. getRemoteName() { return "remote"; } Should these be customized to the Protocol being used? Cheers, – Philip
          Hide
          Doug Cutting added a comment -

          > Do add some JavaDoc per class.

          Done.

          > Can a single RPC server satisfy multiple protocols?

          Yes, that should work. The client and servers protocols are both determined through reflection. So long as the server implements the subset of methods that the client calls then all's well. Avro doesn't validate the protocol name in RPC, just the method name. Dunno if that's a bug or a feature! (I think this is the same loophole that Hadoop RPC uses.)

          > Typo.

          Fixed.

          > AvroTestProtocol is generated code, yes? Or does ReflectData.getProtocol()
          figure it out from reflection and paranamer data?

          The latter. This current patch only supports reflection-based RPC, since it's intended to be a stepping stone for transition from Hadoop RPC, which is reflection-based.

          > Total nit: JUnit prefers the expected argument on the left.

          Fixed.

          > Eclipse tells me this is unused.

          Added a comment.

          > getRemoteName()

          { return "remote"; }

          Good catch! That was a bug. This is used by Avro to cache the remote protocol. It should be the remote host's address. Fixed.

          I also took the opportunity to upgrade from Avro 1.0 to Avro 1.1, which required a few minor changes to the io/serializer/avro stuff.

          Show
          Doug Cutting added a comment - > Do add some JavaDoc per class. Done. > Can a single RPC server satisfy multiple protocols? Yes, that should work. The client and servers protocols are both determined through reflection. So long as the server implements the subset of methods that the client calls then all's well. Avro doesn't validate the protocol name in RPC, just the method name. Dunno if that's a bug or a feature! (I think this is the same loophole that Hadoop RPC uses.) > Typo. Fixed. > AvroTestProtocol is generated code, yes? Or does ReflectData.getProtocol() figure it out from reflection and paranamer data? The latter. This current patch only supports reflection-based RPC, since it's intended to be a stepping stone for transition from Hadoop RPC, which is reflection-based. > Total nit: JUnit prefers the expected argument on the left. Fixed. > Eclipse tells me this is unused. Added a comment. > getRemoteName() { return "remote"; } Good catch! That was a bug. This is used by Avro to cache the remote protocol. It should be the remote host's address. Fixed. I also took the opportunity to upgrade from Avro 1.0 to Avro 1.1, which required a few minor changes to the io/serializer/avro stuff.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12419947/HADOOP-6170.patch
          against trunk revision 816409.

          +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 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 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/45/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/45/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/45/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/45/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/12419947/HADOOP-6170.patch against trunk revision 816409. +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 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 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/45/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/45/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/45/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/45/console This message is automatically generated.
          Hide
          Doug Cutting added a comment -

          Fix Ivy settings to use library.properties for consistency with other dependencies.

          Show
          Doug Cutting added a comment - Fix Ivy settings to use library.properties for consistency with other dependencies.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12420048/HADOOP-6170.patch
          against trunk revision 816703.

          +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 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 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/52/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/52/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/52/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/52/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/12420048/HADOOP-6170.patch against trunk revision 816703. +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 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 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/52/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/52/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/52/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/52/console This message is automatically generated.
          Hide
          Tom White added a comment -

          +1

          Show
          Tom White added a comment - +1
          Hide
          Doug Cutting added a comment -

          I just committed this.

          Show
          Doug Cutting added a comment - I just committed this.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #39 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/39/)
          . Add facility to tunnel Avro RPCs through Hadoop RPCs.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #39 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/39/ ) . Add facility to tunnel Avro RPCs through Hadoop RPCs.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #102 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/102/)
          . Add facility to tunnel Avro RPCs through Hadoop RPCs.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #102 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/102/ ) . Add facility to tunnel Avro RPCs through Hadoop RPCs.
          Hide
          Robert Chansler added a comment -

          Editorial pass over all release notes prior to publication of 0.21.

          Show
          Robert Chansler added a comment - Editorial pass over all release notes prior to publication of 0.21.

            People

            • Assignee:
              Doug Cutting
              Reporter:
              Doug Cutting
            • Votes:
              0 Vote for this issue
              Watchers:
              19 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development