Dfs methods should not throw RemoteException

• Key: HADOOP-2841
• Type: Improvement
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Konstantin Shvachko
• Reporter: Hairong Kuang
• Affects Version/s: 0.16.0
• Fix Version/s: 0.17.0

Description

Dfs should unwrap the RemoteException and throw the real cause of the error to the user. This allows the user to find out the real cause without examining the remote exception. This also allows the user to use the dfs interface without aware of the implementation details (i.e. rpc).

Comments

[Konstantin Shvachko]

This patch introduces unwrapping for AccessControlException and FileNotFoundException in DFSClient
so that there were no more reason for analyze RemoteException in higher level classes.
This was initially submitted under HADOOP-3108.

This patch introduces two unwrapping methods for RemoteException, which unwrap and throw original
exceptions contained inside. There are 2 conditions for successful unwrapping:

1. the original exception should be an IOException and
2. it should have the default constructors.

Otherwise the RemoteException is re-thrown.

The first method throws only exceptions of the specified type.
While the second one unwraps anything. The second can be potentially used as a generic
unwrapping method directly inside RPC. For now I use just the first one for unwrapping
AccessControlException and FileNotFoundException exceptions.

More details about the changes are here
http://issues.apache.org/jira/browse/HADOOP-3108#action_12583010

I think this patch is important for 0.17 because currently in many place we rely on exception message text
to determine which exception was originally thrown, which imo is a totally unacceptable practice.

[Hadoop QA]

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12378838/getNodeNPE.patch
against trunk revision 619744.

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

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

javadoc +1. The javadoc tool did not generate any warning messages.

javac +1. The applied patch does not generate any new javac compiler warnings.

release audit +1. The applied patch does not generate any new release audit warnings.

findbugs +1. The patch does not introduce any new Findbugs warnings.

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

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

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2096/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2096/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2096/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2096/console

This message is automatically generated.

[Hairong Kuang]

Can we unwrap the RemoteException at the ipc level?
When unwrapping RemoteException in DFSClient, I do not think we need to introduce a varaible "result" in every method.
Import "StringUtils" in FsShell is unneccessary.
In src/test/org/apache/hadoop/security/TestPermission.java, canXX methods only need to catch AccessControlException not IOException.

[Hairong Kuang]

RemoteException.unwrapRemoteException methods waste an instantiation of an exception if this exception is not an IOException. Better to use Class.isAssignableFrom to determine the exception class is a subclass of IOException before the instantiation.

[Konstantin Shvachko]

I modified patch per Hairongs comments.
unwrapRemoteException accepets now a variable number of classes to look for unwrapping
and returns an exception rather than throwing it as in the previous patch.
This make DFSClient code less polluted.

[Hadoop QA]

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12379209/getNodeNPE.patch
against trunk revision 643282.

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

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

javadoc -1. The javadoc tool appears to have generated 1 warning messages.

javac +1. The applied patch does not generate any new javac compiler warnings.

release audit +1. The applied patch does not generate any new release audit warnings.

findbugs +1. The patch does not introduce any new Findbugs warnings.

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

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

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2132/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2132/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2132/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2132/console

This message is automatically generated.

[Konstantin Shvachko]

Corrected javaDoc, removed trace stack printing in FsShell, used isAssignableFrom one more time.

[Konstantin Shvachko]

Reflect current trunk.

[Hairong Kuang]

+1 The patch looks good.

[Konstantin Shvachko]

Merging with the trunk again.

[Konstantin Shvachko]

I verified this patch on Windows and Linux. Nigel ran the test-patch target. The results are positive:
exec] +1 overall.
[exec]
[exec] @author +1. The patch does not contain any @author tags.
[exec]
[exec] tests included +1. The patch appears to include 6 new or modified tests.
[exec]
[exec] javadoc +1. The javadoc tool did not generate any warning messages.
[exec]
[exec] javac +1. The applied patch does not generate any new javac compiler warnings.
[exec]
[exec] findbugs +1. The patch does not introduce any new Findbugs warnings.

[Konstantin Shvachko]

I just committed this.

[Hudson]

Integrated in Hadoop-trunk #451 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/451/)