Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.17.1
    • Fix Version/s: 0.19.0
    • Component/s: None
    • Labels:
      None
    • Environment:

      any POSIX

    • Hadoop Flags:
      Reviewed
    • Release Note:
      Improved error reporting for libhdfs so permission problems now return EACCES.

      Description

      A comment near the top of hdfs.h says that "All APIs set errno to meaningful values." Unfortunately this is a big fat lie – bigger and fatter still now that we have permissions in HDFS. If Hadoop throws an AccessControlException, then libhdfs sets errno to EINTERNAL, even though this is not an "internal" error in any sense of the word.

      With the attached patches, allow libhdfs to derive a useful errno value when an exception is thrown. I've implemented handling for AccessControlException and a few others whose semantics seem obvious.

      Tangential question: The signatures for invokeMethod() and constructNewObjectOfClass() in hdfsJniHelper.h disagreed with their comments. Was the "exc" argument of those functions removed in the past, or never implemented? I (re)instated it in my patch.

      1. libhdfs-04.patch
        41 kB
        Ben Slusky
      2. libhdfs-03-combined.patch
        39 kB
        Ben Slusky
      3. libhdfs-02-meaningful-errno.patch
        34 kB
        Ben Slusky
      4. libhdfs-01-exception-handling.patch
        25 kB
        Ben Slusky

        Issue Links

          Activity

          Ben Slusky created issue -
          Ben Slusky made changes -
          Field Original Value New Value
          Status Open [ 1 ] Patch Available [ 10002 ]
          Ben Slusky made changes -
          Attachment libhdfs-01-exception-handling.patch [ 12383927 ]
          Attachment libhdfs-02-meaningful-errno.patch [ 12383928 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12383928/libhdfs-02-meaningful-errno.patch
          against trunk revision 667040.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no tests are needed for this patch.

          -1 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2651/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/12383928/libhdfs-02-meaningful-errno.patch against trunk revision 667040. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2651/console This message is automatically generated.
          Hide
          Ben Slusky added a comment -

          Ok, so layered diffs are bad. Good to know. This is a combined diff against trunk. I removed the translation of DiskOutOfSpaceException to ENOSPC, since DiskOutOfSpaceException is apparently not thrown in the client.

          Testing this automatically is a tricky business. AFAICT all hdfs tests are run as the superuser, so there's no way to trigger EACCES. I had written up a test to trigger ENOSPC, only to find that the exception that gets thrown when you fill up the disk is ambiguous (not to mention different from DFS to local FS).

          Show
          Ben Slusky added a comment - Ok, so layered diffs are bad. Good to know. This is a combined diff against trunk. I removed the translation of DiskOutOfSpaceException to ENOSPC, since DiskOutOfSpaceException is apparently not thrown in the client. Testing this automatically is a tricky business. AFAICT all hdfs tests are run as the superuser, so there's no way to trigger EACCES. I had written up a test to trigger ENOSPC, only to find that the exception that gets thrown when you fill up the disk is ambiguous (not to mention different from DFS to local FS).
          Ben Slusky made changes -
          Attachment libhdfs-03-combined.patch [ 12383981 ]
          Chris Douglas made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Chris Douglas made changes -
          Assignee Ben Slusky [ sluskyb ]
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12383981/libhdfs-03-combined.patch
          against trunk revision 669646.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no tests are needed for this patch.

          +1 javadoc. The javadoc tool did not generate any 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 failed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2701/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2701/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2701/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2701/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/12383981/libhdfs-03-combined.patch against trunk revision 669646. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2701/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2701/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2701/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2701/console This message is automatically generated.
          Ben Slusky made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Ben Slusky made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Affects Version/s 0.17.0 [ 12312913 ]
          Affects Version/s 0.17.1 [ 12313190 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12383981/libhdfs-03-combined.patch
          against trunk revision 677872.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no tests are needed for this patch.

          +1 javadoc. The javadoc tool did not generate any 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.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2903/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2903/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2903/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2903/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/12383981/libhdfs-03-combined.patch against trunk revision 677872. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any 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. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2903/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2903/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2903/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2903/console This message is automatically generated.
          Ben Slusky made changes -
          Link This issue relates to HADOOP-3264 [ HADOOP-3264 ]
          Hide
          Ben Slusky added a comment -

          If libhdfs had a permissions API, then we could make a test that would trigger EACCES.

          Show
          Ben Slusky added a comment - If libhdfs had a permissions API, then we could make a test that would trigger EACCES.
          Ben Slusky made changes -
          Link This issue is related to HADOOP-3536 [ HADOOP-3536 ]
          Hide
          Owen O'Malley added a comment -

          I'm sorry this patch sat so long to review, but I'd rather that the macro CHECK_EXCEPTION_IN_METHOD_INVOC declared the temporary jthr rather than just assuming it exists in the function that includes it. It would have also been better to fix the macro to take the arguments env and exc, although I know it is already using env implicitly.

          Show
          Owen O'Malley added a comment - I'm sorry this patch sat so long to review, but I'd rather that the macro CHECK_EXCEPTION_IN_METHOD_INVOC declared the temporary jthr rather than just assuming it exists in the function that includes it. It would have also been better to fix the macro to take the arguments env and exc, although I know it is already using env implicitly.
          Owen O'Malley made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Ben Slusky made changes -
          Attachment libhdfs-04.patch [ 12388246 ]
          Hide
          Ben Slusky added a comment -

          How about this patch, which eliminates that ugly macro altogether?

          Show
          Ben Slusky added a comment - How about this patch, which eliminates that ugly macro altogether?
          Ben Slusky made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Owen O'Malley added a comment -

          I just committed this. Thanks, Ben!

          Show
          Owen O'Malley added a comment - I just committed this. Thanks, Ben!
          Owen O'Malley made changes -
          Release Note Permission problems now return EACCES in errno from libhdfs.
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hadoop Flags [Reviewed]
          Owen O'Malley made changes -
          Fix Version/s 0.19.0 [ 12313211 ]
          Hide
          Hudson added a comment -
          Show
          Hudson added a comment - Integrated in Hadoop-trunk #581 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/581/ )
          Robert Chansler made changes -
          Release Note Permission problems now return EACCES in errno from libhdfs. Improved error reporting for libhdfs so permission problems now return EACCES.
          Nigel Daley made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Owen O'Malley made changes -
          Component/s libhdfs [ 12311345 ]

            People

            • Assignee:
              Ben Slusky
              Reporter:
              Ben Slusky
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development