Hadoop Common
  1. Hadoop Common
  2. HADOOP-6930

AvroRpcEngine doesn't work with generated Avro code

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.22.0
    • Component/s: ipc
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      AvroRpcEngine uses 'reflect' based java implementation. There should be a way to have it work with 'specific' (generated code from avro idl).

      1. Hadoop-6930.patch
        8 kB
        Sharad Agarwal
      2. Hadoop-6930_3.patch
        14 kB
        Sharad Agarwal
      3. Hadoop-6930_2.patch
        12 kB
        Sharad Agarwal
      4. Hadoop-6930_1.patch
        11 kB
        Sharad Agarwal

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #374 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/374/)
          HADOOP-6930. AvroRpcEngine doesn't work with generated Avro code. Contributed by Sharad Agarwal.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #374 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/374/ ) HADOOP-6930 . AvroRpcEngine doesn't work with generated Avro code. Contributed by Sharad Agarwal.
          Hide
          Sharad Agarwal added a comment -

          I just committed this.

          Show
          Sharad Agarwal added a comment - I just committed this.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12454038/Hadoop-6930_3.patch
          against trunk revision 992479.

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

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

          -1 javadoc. The javadoc tool appears to have generated 1 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.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/670/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/670/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/670/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/670/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/12454038/Hadoop-6930_3.patch against trunk revision 992479. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 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. +1 system tests framework. The patch passed system tests framework compile. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/670/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/670/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/670/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/670/console This message is automatically generated.
          Hide
          Sharad Agarwal added a comment -

          Fixed release audit warnings by adding license header to new files. javadoc warnings are not introduced by this patch.

          Show
          Sharad Agarwal added a comment - Fixed release audit warnings by adding license header to new files. javadoc warnings are not introduced by this patch.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12453696/Hadoop-6930_2.patch
          against trunk revision 992479.

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

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

          -1 javadoc. The javadoc tool appears to have generated 1 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 generated 4 release audit warnings (more than the trunk's current 2 warnings).

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

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/669/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/669/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/669/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/669/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/669/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/12453696/Hadoop-6930_2.patch against trunk revision 992479. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 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 generated 4 release audit warnings (more than the trunk's current 2 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system tests framework. The patch passed system tests framework compile. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/669/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/669/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/669/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/669/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/669/console This message is automatically generated.
          Hide
          Doug Cutting added a comment -

          +1

          Show
          Doug Cutting added a comment - +1
          Hide
          Sharad Agarwal added a comment -

          Incorporated review comments.

          Show
          Sharad Agarwal added a comment - Incorporated review comments.
          Hide
          Doug Cutting added a comment -

          A few minor things:

          • WritableRpcEngine should be made public too; and
          • setProtocolEngine, now public, needs javadoc.
            Other than that, +1
          Show
          Doug Cutting added a comment - A few minor things: WritableRpcEngine should be made public too; and setProtocolEngine, now public, needs javadoc. Other than that, +1
          Hide
          Sharad Agarwal added a comment -

          Thanks Doug for looking at this. Here is the patch which has the missed protocol file. Also I have made the classes as public and marked them evolving.

          Show
          Sharad Agarwal added a comment - Thanks Doug for looking at this. Here is the patch which has the missed protocol file. Also I have made the classes as public and marked them evolving.
          Hide
          Doug Cutting added a comment -

          Looks reasonable, but I think you forgot to include the test protocol file.

          > In order to use these engine, right now I need to be in org.apache.hadoop.ipc package.

          The original idea was that you'd use this by configuring protocols to use particular engines. But more explicit control might be good too. If we do make them public then we'd want to make sure to mark the APIs as evolving, not stable.

          Show
          Doug Cutting added a comment - Looks reasonable, but I think you forgot to include the test protocol file. > In order to use these engine, right now I need to be in org.apache.hadoop.ipc package. The original idea was that you'd use this by configuring protocols to use particular engines. But more explicit control might be good too. If we do make them public then we'd want to make sure to mark the APIs as evolving, not stable.
          Hide
          Sharad Agarwal added a comment -

          Here is the patch adding AvroSpecificRpcEngine.

          Should we make the RpcEngine a public API instead of package private ? In order to use these engine, right now I need to be in org.apache.hadoop.ipc package.

          Show
          Sharad Agarwal added a comment - Here is the patch adding AvroSpecificRpcEngine. Should we make the RpcEngine a public API instead of package private ? In order to use these engine, right now I need to be in org.apache.hadoop.ipc package.
          Hide
          Doug Cutting added a comment -

          > Here clients HAVE to choose between 'reflect' and 'specific' Avro engine, but that should be ok for now.

          That would work if you only transition protocols to Specific and never wish to rely on Reflect. Since some parameter types are used by multiple protocols, this requires you to transition more (all?) protocols at once, whereas, if some parameters could use Reflect, you could transition things from Writable to Avro protocol-by-protocol, which might be easier to debug. But AvroSpecificRpcEngine should be easy to write, and, long-term, when all protocols use Specific, we'll want this, so it'd be a good addition. +1

          Show
          Doug Cutting added a comment - > Here clients HAVE to choose between 'reflect' and 'specific' Avro engine, but that should be ok for now. That would work if you only transition protocols to Specific and never wish to rely on Reflect. Since some parameter types are used by multiple protocols, this requires you to transition more (all?) protocols at once, whereas, if some parameters could use Reflect, you could transition things from Writable to Avro protocol-by-protocol, which might be easier to debug. But AvroSpecificRpcEngine should be easy to write, and, long-term, when all protocols use Specific, we'll want this, so it'd be a good addition. +1
          Hide
          Sharad Agarwal added a comment -

          Having 'reflect' and 'specific' types as interoperable would be a great thing. But for now I am thinking of a simpler fix -> Have AvroSpecificRpcEngine (that extends AvroRpcEngine) and let AvroRpcEngine to have protected factory methods for creating Requestor and Responder.
          Here clients HAVE to choose between 'reflect' and 'specific' Avro engine, but that should be ok for now.

          Show
          Sharad Agarwal added a comment - Having 'reflect' and 'specific' types as interoperable would be a great thing. But for now I am thinking of a simpler fix -> Have AvroSpecificRpcEngine (that extends AvroRpcEngine) and let AvroRpcEngine to have protected factory methods for creating Requestor and Responder. Here clients HAVE to choose between 'reflect' and 'specific' Avro engine, but that should be ok for now.
          Hide
          Doug Cutting added a comment -

          Avro 1.4, which should be released next week, better integrates 'specific' and 'reflect'. With a little more effort this could be make to 'just work', i.e., ReflectRequestor and ReflectResponder would be able to handle Specific data. Glancing at the code, it looks like mostly what remains is fixing a few isX() methods in ReflectData so that unions would be dispatched correctly. Making GenericArray to implement Collection would also simplify this. Anyway, it's close. If this sounds of interest, please file an Avro Jira requesting this.

          Alternately, we could simply configure things, protocol-by-protocol to use either specific or reflect. But intermixing might then cause problems, and it might be nice to be able to move more incrementally.

          Show
          Doug Cutting added a comment - Avro 1.4, which should be released next week, better integrates 'specific' and 'reflect'. With a little more effort this could be make to 'just work', i.e., ReflectRequestor and ReflectResponder would be able to handle Specific data. Glancing at the code, it looks like mostly what remains is fixing a few isX() methods in ReflectData so that unions would be dispatched correctly. Making GenericArray to implement Collection would also simplify this. Anyway, it's close. If this sounds of interest, please file an Avro Jira requesting this. Alternately, we could simply configure things, protocol-by-protocol to use either specific or reflect. But intermixing might then cause problems, and it might be nice to be able to move more incrementally.

            People

            • Assignee:
              Sharad Agarwal
              Reporter:
              Sharad Agarwal
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development