Hadoop Common
  1. Hadoop Common
  2. HADOOP-6900

FileSystem#listLocatedStatus should not throw generic RuntimeException to indicate error conditions

    Details

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

      Description

      HDFS-6870 introduced FileSystem#listLocatedStatus(), that returns an Iterator to iterate through LocatedFileStatus for files under a directory or recursively under a sub-directory. Iterator currently throws generic RuntimeException to indicate error conditions. API needs to be changed to throw appropriate exceptions to indicate error conditions.

      1. listFilesIO.patch
        20 kB
        Hairong Kuang
      2. listFilesIO1.patch
        25 kB
        Hairong Kuang

        Issue Links

          Activity

          Hide
          Suresh Srinivas added a comment -

          Iterator#hasNext() has to indicate the following error conditions:

          1. next element exists - returns true
          2. next element does not exist - returns false
          3. could not get status for the next element, because a directory that was available before iteration started does not exist any more.
          4. could not talk to the remote server to get status.
          5. remote server throws IOException due to an error while getting the status.

          For the last 3 conditions, currently RuntimeException is thrown. Throwing RTE forces application to catch it, to handle the error condition. This results in application catching other RTE as well. Worse still, an application that does not catch RTE might exit.

          The problem is, Java Iterator interface is not suitable for implementation that use RPC underneath. We should have a different interface, which could indicate error conditions more clearly.

          Additionally I feel when a directory gets deleted in the middle of iteration, we should ignore it, or add provision to the iterator to ignore it.

          Show
          Suresh Srinivas added a comment - Iterator#hasNext() has to indicate the following error conditions: next element exists - returns true next element does not exist - returns false could not get status for the next element, because a directory that was available before iteration started does not exist any more. could not talk to the remote server to get status. remote server throws IOException due to an error while getting the status. For the last 3 conditions, currently RuntimeException is thrown. Throwing RTE forces application to catch it, to handle the error condition. This results in application catching other RTE as well. Worse still, an application that does not catch RTE might exit. The problem is, Java Iterator interface is not suitable for implementation that use RPC underneath. We should have a different interface, which could indicate error conditions more clearly. Additionally I feel when a directory gets deleted in the middle of iteration, we should ignore it, or add provision to the iterator to ignore it.
          Hide
          Hairong Kuang added a comment -

          Suresh, good idea! I appreciate your thoughts on this.

          I will create a new Iterator interface that throws IOException on next() and hasNext().

          I will make listFiles, listLocatedStatus, AND listStatus to return this new iterator iterface. How does this sound to you?

          Show
          Hairong Kuang added a comment - Suresh, good idea! I appreciate your thoughts on this. I will create a new Iterator interface that throws IOException on next() and hasNext(). I will make listFiles, listLocatedStatus, AND listStatus to return this new iterator iterface. How does this sound to you?
          Hide
          Hairong Kuang added a comment -

          A patch for review.

          Show
          Hairong Kuang added a comment - A patch for review.
          Hide
          Hairong Kuang added a comment -

          > Additionally I feel when a directory gets deleted in the middle of iteration, we should ignore it, or add provision to the iterator to ignore it.

          Unfortunately not allowing concurrent modification on the input directories is the semantics that map/reduce has it now. I do not want to introduce an incompatible change for now.

          Show
          Hairong Kuang added a comment - > Additionally I feel when a directory gets deleted in the middle of iteration, we should ignore it, or add provision to the iterator to ignore it. Unfortunately not allowing concurrent modification on the input directories is the semantics that map/reduce has it now. I do not want to introduce an incompatible change for now.
          Hide
          Hairong Kuang added a comment -

          This patch adds more comments and changes some FileContext tests to use the new listStatus interface.

          Show
          Hairong Kuang added a comment - This patch adds more comments and changes some FileContext tests to use the new listStatus interface.
          Hide
          Suresh Srinivas added a comment -

          + 1 for the patch.

          Unfortunately not allowing concurrent modification on the input directories is the semantics that map/reduce has it now. I do not want to introduce an incompatible change for now.

          Given that this is a new API, I am not sure handling concurrent modification and ignoring deleted directories is a real backward compatibility issue for mapreduce. Mapreduce folks can comment about what is more suitable. We can address this issue in a separate jira though.

          Show
          Suresh Srinivas added a comment - + 1 for the patch. Unfortunately not allowing concurrent modification on the input directories is the semantics that map/reduce has it now. I do not want to introduce an incompatible change for now. Given that this is a new API, I am not sure handling concurrent modification and ignoring deleted directories is a real backward compatibility issue for mapreduce. Mapreduce folks can comment about what is more suitable. We can address this issue in a separate jira though.
          Hide
          Sanjay Radia added a comment -

          >I do not want to introduce an incompatible change for now.
          I don't understand the compatibility argument since this is new API.

          We could add a parameter to indicate whether or not continue if a change in the sub-tree has occurred.
          I can live with that as long as folks not interpret it to mean that the list returned is atomic - if a subdir's content has been
          returned then the iterator will not detect it.
          Hairong's patch can go though as Suresh has suggested and we can file another one to finalize this API.

          Show
          Sanjay Radia added a comment - >I do not want to introduce an incompatible change for now. I don't understand the compatibility argument since this is new API. We could add a parameter to indicate whether or not continue if a change in the sub-tree has occurred. I can live with that as long as folks not interpret it to mean that the list returned is atomic - if a subdir's content has been returned then the iterator will not detect it. Hairong's patch can go though as Suresh has suggested and we can file another one to finalize this API.
          Hide
          Hairong Kuang added a comment -

          > I don't understand the compatibility argument since this is new API.

          Even though this is a new API, but mapreduce already has some logic in their code traversing the input directories and getting file status/block locations. Existing code does not allow any concurrent modification to input directories.

          I agree that allowing concurrent modification could be an option, but lets do it in a different jira.

          Show
          Hairong Kuang added a comment - > I don't understand the compatibility argument since this is new API. Even though this is a new API, but mapreduce already has some logic in their code traversing the input directories and getting file status/block locations. Existing code does not allow any concurrent modification to input directories. I agree that allowing concurrent modification could be an option, but lets do it in a different jira.
          Hide
          Hairong Kuang added a comment -

          Ant test-patch result:

          [exec] -1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 12 new or modified tests.
          [exec]
          [exec] -1 javadoc. The javadoc tool appears to have generated 1 warning messages.
          [exec]
          [exec] +1 javac.
          [exec] The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec]
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

          Javadoc warnings are not related to this patch.

          Ant test:
          BUILD SUCCESSFUL
          Total time: 11 minutes 2 seconds

          Show
          Hairong Kuang added a comment - Ant test-patch result: [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 12 new or modified tests. [exec] [exec] -1 javadoc. The javadoc tool appears to have generated 1 warning messages. [exec] [exec] +1 javac. [exec] The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. Javadoc warnings are not related to this patch. Ant test: BUILD SUCCESSFUL Total time: 11 minutes 2 seconds
          Hide
          Hairong Kuang added a comment -

          I've just committed this!

          Show
          Hairong Kuang added a comment - I've just committed this!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #356 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/356/)
          HADOOP-6900. Make the iterator returned by FileSystem#listLocatedStatus to throw IOException rather than RuntimeException when there is an IO error fetching the next file. Contributed by Hairong Kuang.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #356 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/356/ ) HADOOP-6900 . Make the iterator returned by FileSystem#listLocatedStatus to throw IOException rather than RuntimeException when there is an IO error fetching the next file. Contributed by Hairong Kuang.
          Hide
          Konstantin Shvachko added a comment -

          Hairong. You changed the prototype of AbstractFileSystem.listStatusIterator(Path) but did not change its overloaded version Hdfs.listStatusIterator(Path). Could you please fix it, as current build is broken as it is.

          Show
          Konstantin Shvachko added a comment - Hairong. You changed the prototype of AbstractFileSystem.listStatusIterator(Path) but did not change its overloaded version Hdfs.listStatusIterator(Path) . Could you please fix it, as current build is broken as it is.
          Hide
          Hairong Kuang added a comment -

          Yes, I can not change HDFS in common but have to do it in HDFS-202. This is side effect of having fs in common.

          Show
          Hairong Kuang added a comment - Yes, I can not change HDFS in common but have to do it in HDFS-202 . This is side effect of having fs in common.

            People

            • Assignee:
              Hairong Kuang
              Reporter:
              Suresh Srinivas
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development