Details

Description

YarnRemoteException is defined as a generic wrapper for all the exceptions in yarn. I think we should instead throw IOExceptions in the API, which can later be extended for more specialized exceptions without breaking compatibility.

Activity

YarnRemoteException was serializing the actual exception and sending it over the wire. It was missing an unwind method to get the actual exception. IAC - clients should not be expected to unwind and that should've been handled in the RPC layer itself.

Instead of having the APIs throw an IOException, I'd prefer to define a Yarn / HadoopException which extends Exception and can later be sub-classed for more specific exceptions. Also, possibly add error codes to these exceptions as well as to what is sent over the wire. That may make things easier for clients written in other languages.

Siddharth Seth
added a comment - 30/Mar/12 22:37 YarnRemoteException was serializing the actual exception and sending it over the wire. It was missing an unwind method to get the actual exception. IAC - clients should not be expected to unwind and that should've been handled in the RPC layer itself.
Instead of having the APIs throw an IOException, I'd prefer to define a Yarn / HadoopException which extends Exception and can later be sub-classed for more specific exceptions. Also, possibly add error codes to these exceptions as well as to what is sent over the wire. That may make things easier for clients written in other languages.

Siddharth Seth
added a comment - 07/May/12 19:47 Turns out the RPC/Security layers don't handle anything other than IOException very well (ugi.doAs will result in UndeclaredThrowableException if not an IOException).
Changes in the patch
All APIs also throw IOException (for exception originating in the RPC layer)
YarnRemoteException (continues to extend IOE) can be extended for custom exceptions.
The old YarnRemoteExceptionPBImpl moved as a record to SerializedException (used by some yarn APIs)
Added some custom exceptions like UnknownAppId
If folks are ok with this patch, would like to rename YarnRemoteException to YarnException, and the existing YarnException to YarnRuntimeException before this is checked in.

Siddharth Seth
added a comment - 03/Oct/12 21:33 Cancelling, the patch will need a rebase.
This is a blocker for a beta release - since it changes APIs. For now leaving it as is.
Craeted YARN-142 to take care of the YARN changes.

I think we are on the correct patch. We should just replace the YarnRemoteException with IOException in all of MR protocols. Clearly the legacy MR clients don't know or care about YarnRemoteExceptions.

Vinod Kumar Vavilapalli
added a comment - 08/May/13 20:58 Revisiting this.
I think we are on the correct patch. We should just replace the YarnRemoteException with IOException in all of MR protocols. Clearly the legacy MR clients don't know or care about YarnRemoteExceptions.

1. Change MRClientProtocol API to throw IOException instead of YarnRemoteException
2. In MRClientProtocolPBClientImpl, instead of using RPCUtil.unwrap method to unwrap and throw exceptions, we create new unwrap method to throw IOException.
3. All MR apis will not throw YarnRemoteException anymore, they will wrap YarnRemoteException and throw out IOException instead

Vinod Kumar Vavilapalli
added a comment - 13/May/13 04:26 The patch looks good to me, except for a System.out.println in TestMRJobClient, which isn't bad.
+1. Will do a quick fresh-compilation and check this in.