Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-6930

AvroRpcEngine doesn't work with generated Avro code

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 0.22.0
    • ipc
    • None
    • Reviewed

    Description

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

      Attachments

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

        Issue Links

          Activity

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

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

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

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

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

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

            sharadag 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.
            cutting 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
            cutting 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
            sharadag Sharad Agarwal added a comment -

            Incorporated review comments.

            sharadag Sharad Agarwal added a comment - Incorporated review comments.
            cutting Doug Cutting added a comment -

            +1

            cutting Doug Cutting added a comment - +1
            hadoopqa 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.

            hadoopqa 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.
            sharadag Sharad Agarwal added a comment -

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

            sharadag Sharad Agarwal added a comment - Fixed release audit warnings by adding license header to new files. javadoc warnings are not introduced by this patch.
            hadoopqa 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.

            hadoopqa 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.
            sharadag Sharad Agarwal added a comment -

            I just committed this.

            sharadag Sharad Agarwal added a comment - I just committed this.
            hudson 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.

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

            People

              sharadag Sharad Agarwal
              sharadag Sharad Agarwal
              Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: