Hadoop Common
  1. Hadoop Common
  2. HADOOP-2841

Dfs methods should not throw RemoteException

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.16.0
    • Fix Version/s: 0.17.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

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

      1. getNodeNPE.patch
        24 kB
        Konstantin Shvachko
      2. getNodeNPE.patch
        24 kB
        Konstantin Shvachko
      3. getNodeNPE.patch
        24 kB
        Konstantin Shvachko
      4. getNodeNPE.patch
        23 kB
        Konstantin Shvachko
      5. getNodeNPE.patch
        23 kB
        Konstantin Shvachko
      6. getNodeNPE.patch
        23 kB
        Konstantin Shvachko

        Issue Links

          Activity

          Hide
          Konstantin Shvachko added a comment -

          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.

          Show
          Konstantin Shvachko added a comment - 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: the original exception should be an IOException and 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.
          Hide
          Hadoop QA added a comment -

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

          Show
          Hadoop QA added a comment - +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.
          Hide
          Hairong Kuang added a comment - - edited

          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.

          Show
          Hairong Kuang added a comment - - edited 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.
          Hide
          Hairong Kuang added a comment -

          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.

          Show
          Hairong Kuang added a comment - 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.
          Hide
          Konstantin Shvachko added a comment -

          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.

          Show
          Konstantin Shvachko added a comment - 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.
          Hide
          Hadoop QA added a comment -

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

          Show
          Hadoop QA added a comment - -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.
          Hide
          Konstantin Shvachko added a comment -

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

          Show
          Konstantin Shvachko added a comment - Corrected javaDoc, removed trace stack printing in FsShell, used isAssignableFrom one more time.
          Hide
          Konstantin Shvachko added a comment -

          Reflect current trunk.

          Show
          Konstantin Shvachko added a comment - Reflect current trunk.
          Hide
          Hairong Kuang added a comment -

          +1 The patch looks good.

          Show
          Hairong Kuang added a comment - +1 The patch looks good.
          Hide
          Konstantin Shvachko added a comment -

          Merging with the trunk again.

          Show
          Konstantin Shvachko added a comment - Merging with the trunk again.
          Hide
          Konstantin Shvachko added a comment -

          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.

          Show
          Konstantin Shvachko added a comment - 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.
          Hide
          Konstantin Shvachko added a comment -

          I just committed this.

          Show
          Konstantin Shvachko added a comment - I just committed this.
          Hide
          Hudson added a comment -
          Show
          Hudson added a comment - Integrated in Hadoop-trunk #451 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/451/ )

            People

            • Assignee:
              Konstantin Shvachko
              Reporter:
              Hairong Kuang
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development