Avro
  1. Avro
  2. AVRO-732

Generated protocol's method should not throw AvroRemoteException

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 1.8.0
    • Component/s: java
    • Labels:
      None

      Description

      If user does NOT define the throws clause in the idl, the code is generated with "throws AvroRemoteException" clause. However on throwing the AvroRemoteException from the implementation, the serialization fails. This is not intuitive to users.

        Activity

        Hide
        Sharad Agarwal added a comment -

        More details and Example.
        a) For idl:
        void test();
        Generated code is:
        public void test() throws AvroRemoteException;

        b) For idl:
        void test1() throws TestError;
        Generated code is:
        public void test1() throws AvroRemoteException, TestException;

        The generated code should add the throws clause only for the errors defined in idl.

        For above example the generated code should be:
        a) public void test();
        b) public void test1() throws TestException;

        Show
        Sharad Agarwal added a comment - More details and Example. a) For idl: void test(); Generated code is: public void test() throws AvroRemoteException; b) For idl: void test1() throws TestError; Generated code is: public void test1() throws AvroRemoteException, TestException; The generated code should add the throws clause only for the errors defined in idl. For above example the generated code should be: a) public void test(); b) public void test1() throws TestException;
        Hide
        Doug Cutting added a comment -

        This sounds like it would be a good improvement. I can't recall why I did things this way. It looks to be a mistake.

        Show
        Doug Cutting added a comment - This sounds like it would be a good improvement. I can't recall why I did things this way. It looks to be a mistake.
        Hide
        Scott Carey added a comment -

        The code always adds AvroRemoteException if is not a one-way message:

        in the compiler's velocity template protocol.vm there is:

        )#if (! $message.isOneWay())
        throws org.apache.avro.AvroRemoteException##

        and corresponding logic in the Avro14SpecificCompiler test class.

        Is it OK to remove this? Or is the suggestion that if an exception is declared, then omit the AvroRemoteException? If we get rid of it completely, what is the expected behavior when the remote side sends back an error? I'm not familiar enough with the protocols to fix this, but we should do so for 1.5.0 since the exception signature is changing due to AVRO-737.

        Show
        Scott Carey added a comment - The code always adds AvroRemoteException if is not a one-way message: in the compiler's velocity template protocol.vm there is: )#if (! $message.isOneWay()) throws org.apache.avro.AvroRemoteException## and corresponding logic in the Avro14SpecificCompiler test class. Is it OK to remove this? Or is the suggestion that if an exception is declared, then omit the AvroRemoteException? If we get rid of it completely, what is the expected behavior when the remote side sends back an error? I'm not familiar enough with the protocols to fix this, but we should do so for 1.5.0 since the exception signature is changing due to AVRO-737 .
        Hide
        Holger Hoffstätte added a comment -

        My vote would be to:

        • get rid of AvroRemoteException in interfaces
        • serialize service-level remote exceptions & pass them through (most exceptions are serializable)
        • throw AvroRemoteExceptions for caught non-RuntimeExceptions, i.e. transport errors as cause

        AvroRemoteException should therefore not be a subclass of IOException.

        This allows clients and any client-side infrastructure (proxy interceptors etc.) to distinguish between interface-declared service exceptions and undeclared transport exceptions via AvroRemoteExceptions with a cause.

        Show
        Holger Hoffstätte added a comment - My vote would be to: get rid of AvroRemoteException in interfaces serialize service-level remote exceptions & pass them through (most exceptions are serializable) throw AvroRemoteExceptions for caught non-RuntimeExceptions, i.e. transport errors as cause AvroRemoteException should therefore not be a subclass of IOException. This allows clients and any client-side infrastructure (proxy interceptors etc.) to distinguish between interface-declared service exceptions and undeclared transport exceptions via AvroRemoteExceptions with a cause.
        Hide
        Philip Zeyliger added a comment -

        Serializing Java exceptions only works for Java: I think you've more or less got to coerce the exception into a string.

        Show
        Philip Zeyliger added a comment - Serializing Java exceptions only works for Java: I think you've more or less got to coerce the exception into a string.
        Hide
        Holger Hoffstätte added a comment -

        Philip: you are of course completely correct - thanks for pointing it out. This means that every implementation needs to be aware of exceptional return values in general, and at least understand the returned "special" payload enough to extract meaningful information from it.
        I guess this brings us to the question whether we should force lowest-common-denominator behaviour on all languages, or allow semantic differences & capabilities to shine through.

        Show
        Holger Hoffstätte added a comment - Philip: you are of course completely correct - thanks for pointing it out. This means that every implementation needs to be aware of exceptional return values in general, and at least understand the returned "special" payload enough to extract meaningful information from it. I guess this brings us to the question whether we should force lowest-common-denominator behaviour on all languages, or allow semantic differences & capabilities to shine through.
        Hide
        Doug Cutting added a comment -

        For specific and reflect code, the natural mapping to me is that errors declared in the protocol should be declared as thrown in the generated interface methods. Generic code needs to throw something that's both an exception and contains GenericRecord. Currently this is done by wrapping a GenericData.Record within a AvroRemoteException.

        I am still +1 for removing AvroRemoteException from the list of exceptions thrown by generated code. Perhaps other exception-related improvements could be made, but I'd prefer to limit this issue to that improvement if possible.

        Show
        Doug Cutting added a comment - For specific and reflect code, the natural mapping to me is that errors declared in the protocol should be declared as thrown in the generated interface methods. Generic code needs to throw something that's both an exception and contains GenericRecord. Currently this is done by wrapping a GenericData.Record within a AvroRemoteException. I am still +1 for removing AvroRemoteException from the list of exceptions thrown by generated code. Perhaps other exception-related improvements could be made, but I'd prefer to limit this issue to that improvement if possible.
        Hide
        Doug Cutting added a comment -

        This is an incompatible change to generated interfaces, so we'll schedule it for 1.6.0.

        Show
        Doug Cutting added a comment - This is an incompatible change to generated interfaces, so we'll schedule it for 1.6.0.
        Hide
        Tino Kissig added a comment -

        This issue is still present in 1.7.2. Are there any plans to do this improvement in one of the next releases?

        Show
        Tino Kissig added a comment - This issue is still present in 1.7.2. Are there any plans to do this improvement in one of the next releases?
        Hide
        Doug Cutting added a comment -

        Tino> Are there any plans to do this improvement in one of the next releases?

        We just need a patch and some tests. This could go into 1.8.0. I will not have time to work on this for the next few weeks.

        Show
        Doug Cutting added a comment - Tino> Are there any plans to do this improvement in one of the next releases? We just need a patch and some tests. This could go into 1.8.0. I will not have time to work on this for the next few weeks.
        Hide
        Sébastien Launay added a comment -

        I attached a patch for removing the AvroRemoteException from the generated code than can be found here as well:
        https://github.com/slaunay/avro/commits/enhancement/AVRO-732-remove-generated-remote-exception

        I changed AvroRemoteException parent class from IOException to RuntimeException in order to still be able to catch it while keeping the code compiling.
        I don't know if this is the best approach but at least we have a base for going forward now.

        Show
        Sébastien Launay added a comment - I attached a patch for removing the AvroRemoteException from the generated code than can be found here as well: https://github.com/slaunay/avro/commits/enhancement/AVRO-732-remove-generated-remote-exception I changed AvroRemoteException parent class from IOException to RuntimeException in order to still be able to catch it while keeping the code compiling. I don't know if this is the best approach but at least we have a base for going forward now.
        Hide
        Doug Cutting added a comment -

        Using a RuntimeException for undeclared errors is an improvement. But we should perhaps also split the uses of AvroRemoteException into more than one class. Its uses include:

        • undeclared errors
        • a base class for declared errors
        • a wrapper for error data in the generic API

        We might keep AvroRemoteException for the second use, add a new RuntimeException named AvroUndeclaredException, a new GenericException extending AvroRemoteException that's used by the Generic API. Is that overkill? Can you think of a different re-factoring might break less user code?

        Show
        Doug Cutting added a comment - Using a RuntimeException for undeclared errors is an improvement. But we should perhaps also split the uses of AvroRemoteException into more than one class. Its uses include: undeclared errors a base class for declared errors a wrapper for error data in the generic API We might keep AvroRemoteException for the second use, add a new RuntimeException named AvroUndeclaredException, a new GenericException extending AvroRemoteException that's used by the Generic API. Is that overkill? Can you think of a different re-factoring might break less user code?
        Hide
        Sébastien Launay added a comment -

        I made the following modifications:

        • AvroRemoteException is now a subclass of Exception
        • Generic & Specific still uses AvroRemoteException for declared exception
        • Reflect does not use AvroRemoteException at all
        • every requestor/responder undeclared error is now wrapped into an AvroRuntimeException
        • Requestor API has been changed to throw AvroRemoteException, IOException and implicitly AvroRuntimeException

        This breaks user code for sure but it is not too bad I think.

        You can find the new modifications in an attached squashed commit or here:
        https://github.com/slaunay/avro/commits/enhancement/AVRO-732-remove-generated-remote-exception

        Let me know what you think and if altering the Requestor API to have something clearer but not backward compatible is good idea.

        Show
        Sébastien Launay added a comment - I made the following modifications: AvroRemoteException is now a subclass of Exception Generic & Specific still uses AvroRemoteException for declared exception Reflect does not use AvroRemoteException at all every requestor/responder undeclared error is now wrapped into an AvroRuntimeException Requestor API has been changed to throw AvroRemoteException , IOException and implicitly AvroRuntimeException This breaks user code for sure but it is not too bad I think. You can find the new modifications in an attached squashed commit or here: https://github.com/slaunay/avro/commits/enhancement/AVRO-732-remove-generated-remote-exception Let me know what you think and if altering the Requestor API to have something clearer but not backward compatible is good idea.
        Hide
        Doug Cutting added a comment -

        I'm now starting to worry that switching to an unchecked exception might make it more difficult to write distributed applications. Remote procedure calls are more likely to fail in than local procedure calls and those failures are often recoverable. Distributed applications often explicitly consider the failure of every remote call. Every RPC involves i/o, so always throwing some IOException might be reasonable.

        Does that make any sense? What is the bug we're trying to fix here?

        Show
        Doug Cutting added a comment - I'm now starting to worry that switching to an unchecked exception might make it more difficult to write distributed applications. Remote procedure calls are more likely to fail in than local procedure calls and those failures are often recoverable. Distributed applications often explicitly consider the failure of every remote call. Every RPC involves i/o, so always throwing some IOException might be reasonable. Does that make any sense? What is the bug we're trying to fix here?
        Hide
        Sébastien Launay added a comment -

        I understand your concern but if you stand from the responder point of view you don't want to have a transport exception when implementing your interface.
        When are you supposed to throw a AvroRemoteException or IOException, actually never I think because these exceptions comes from the transport layer not the business layer (except maybe some not wanted runtime exceptions like NPE but you want the framework to translate those automatically I guess).

        On the other hand when using the requestor you want to catch transport errors and differentiate them with business errors indeed. This can be achieved easily with the generic API where you will have documented access to these transport exceptions but not with specific or reflect API because you use a Java interface without those exceptions explicitly declared.

        It's common practice to use a POJO business interface (not tied to the transport layer) for exporting services like Spring does:
        http://static.springsource.org/spring/docs/3.2.x/spring-framework-reference/html/remoting.html

        I agree that catching an undocumented (from the business interface definition) runtime exception when calling a method is not straightforward but that is also the beauty of it where you can implement a retry logic in a higher level (e.g. where you manage the transaction state) or at this very level through an aspect.
        Existing AvroRemoteException in methods is viral because you need to declare that exception recursively in the higher levels if you want to manage it there.
        I guess most people don't add the AvroRemoteException to every methods in there interface either when using the reflect API (not the recommended one though) where it is mandatory today in the specific API.

        Show
        Sébastien Launay added a comment - I understand your concern but if you stand from the responder point of view you don't want to have a transport exception when implementing your interface. When are you supposed to throw a AvroRemoteException or IOException, actually never I think because these exceptions comes from the transport layer not the business layer (except maybe some not wanted runtime exceptions like NPE but you want the framework to translate those automatically I guess). On the other hand when using the requestor you want to catch transport errors and differentiate them with business errors indeed. This can be achieved easily with the generic API where you will have documented access to these transport exceptions but not with specific or reflect API because you use a Java interface without those exceptions explicitly declared. It's common practice to use a POJO business interface (not tied to the transport layer) for exporting services like Spring does: http://static.springsource.org/spring/docs/3.2.x/spring-framework-reference/html/remoting.html I agree that catching an undocumented (from the business interface definition) runtime exception when calling a method is not straightforward but that is also the beauty of it where you can implement a retry logic in a higher level (e.g. where you manage the transaction state) or at this very level through an aspect. Existing AvroRemoteException in methods is viral because you need to declare that exception recursively in the higher levels if you want to manage it there. I guess most people don't add the AvroRemoteException to every methods in there interface either when using the reflect API (not the recommended one though) where it is mandatory today in the specific API.
        Hide
        Doug Cutting added a comment -

        > you don't want to have a transport exception when implementing your interface

        You don't have to throw or even declare every exception in an interface that you implement. Still, having it in the interface may be confusing. Generating two interfaces, one for remote clients and one for implementors, like Spring, would be cleaner. Or we might just improve the documentation around this.

        > AvroRemoteException in methods is viral [ on clients ]

        This is like IOException. A checked exception for transport errors seems appropriate on the client.

        Show
        Doug Cutting added a comment - > you don't want to have a transport exception when implementing your interface You don't have to throw or even declare every exception in an interface that you implement. Still, having it in the interface may be confusing. Generating two interfaces, one for remote clients and one for implementors, like Spring, would be cleaner. Or we might just improve the documentation around this. > AvroRemoteException in methods is viral [ on clients ] This is like IOException. A checked exception for transport errors seems appropriate on the client.
        Hide
        Sébastien Launay added a comment -

        The two interfaces looks cleaner indeed plus we could remove the callback structure on the responder side but it could be a bit confusing for developers to know the one to use I guess.
        This approach is actually used in Spring for RMI and the implementation of JAX-RPC, two somewhat deprecated technologies.

        The recommended technologies by Spring (JAX-RS, JMS, Burlap, Hessian, ...) use the following runtime exception not declared in a single business interface defined by the user:
        https://github.com/SpringSource/spring-framework/blob/v3.2.0.RELEASE/spring-context/src/main/java/org/springframework/remoting/RemoteAccessException.java

        A client object simply receives an implementation for the interface that
        it needs via a bean reference, like it does for a local bean as well.

        A client may catch RemoteAccessException if it wants to, but as
        remote access errors are typically unrecoverable, it will probably let
        such exceptions propagate to a higher level that handles them generically.
        In this case, the client code doesn't show any signs of being involved in
        remote access, as there aren't any remoting-specific dependencies.

        They insist on the unrecoverable error aspect of such remoting interfaces and they are known to be advocates of RuntimeException especially through their famous template pattern.
        I guess these remoting facilities are more targeted towards J2EE applications rather than highly available distributed applications where you expect things to fail and need explicit control.

        The two interfaces available might actually be the best of both worlds, I am wondering if we could use both of them on the client side with the same *Requestor code.
        That is by introspecting the interface's method exceptions in the InvocationHandler we can choose whether to throw an AvroRemoteException if declared or else an AvroRuntimeException.
        We can even choose at generation if the user want to have both interfaces or else just the "business" one or else the current one for backward compatibility (might be confusing again if not properly documented).
        The performance impact should be negligible especially if the behaviour is cached per method.

        Today using ReflectResponder will wrap a transport error into an AvroRemoteException except if it is a runtime or a declared exception.
        This means that if somebody uses an interface without AvroRemoteException declared in the methods, the only way to recover is to catch Exception and check it is an AvroRemoteException because catching it directly AvroRemoteException won't compile.

        Show
        Sébastien Launay added a comment - The two interfaces looks cleaner indeed plus we could remove the callback structure on the responder side but it could be a bit confusing for developers to know the one to use I guess. This approach is actually used in Spring for RMI and the implementation of JAX-RPC, two somewhat deprecated technologies. The recommended technologies by Spring (JAX-RS, JMS, Burlap, Hessian, ...) use the following runtime exception not declared in a single business interface defined by the user: https://github.com/SpringSource/spring-framework/blob/v3.2.0.RELEASE/spring-context/src/main/java/org/springframework/remoting/RemoteAccessException.java A client object simply receives an implementation for the interface that it needs via a bean reference, like it does for a local bean as well. A client may catch RemoteAccessException if it wants to, but as remote access errors are typically unrecoverable, it will probably let such exceptions propagate to a higher level that handles them generically. In this case, the client code doesn't show any signs of being involved in remote access, as there aren't any remoting-specific dependencies. They insist on the unrecoverable error aspect of such remoting interfaces and they are known to be advocates of RuntimeException especially through their famous template pattern. I guess these remoting facilities are more targeted towards J2EE applications rather than highly available distributed applications where you expect things to fail and need explicit control. The two interfaces available might actually be the best of both worlds, I am wondering if we could use both of them on the client side with the same *Requestor code. That is by introspecting the interface's method exceptions in the InvocationHandler we can choose whether to throw an AvroRemoteException if declared or else an AvroRuntimeException. We can even choose at generation if the user want to have both interfaces or else just the "business" one or else the current one for backward compatibility (might be confusing again if not properly documented). The performance impact should be negligible especially if the behaviour is cached per method. Today using ReflectResponder will wrap a transport error into an AvroRemoteException except if it is a runtime or a declared exception. This means that if somebody uses an interface without AvroRemoteException declared in the methods, the only way to recover is to catch Exception and check it is an AvroRemoteException because catching it directly AvroRemoteException won't compile.

          People

          • Assignee:
            Unassigned
            Reporter:
            Sharad Agarwal
          • Votes:
            3 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development