Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-2117

DiskChecker#mkdirsWithExistsAndPermissionCheck may return true even when the dir is not created

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.20.203.1
    • Fix Version/s: 0.20.205.0
    • Component/s: datanode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      In branch-0.20-security as part of HADOOP-6566, DiskChecker#mkdirsWithExistsAndPermissionCheck will return true even if it wasn't able to create the directory, which means instead of throwing a DiskErrorException the code will proceed to getFileStatus and throw a FNF exception. Post HADOOP-7040, which modified makeInstance to catch not just DiskErrorExceptions but IOExceptions as well, this is not an issue since now the exception is caught either way. But for future modifications we should still modify this method to return false if it was unable to create the directory. This code is totally different in trunk, so not an issue there.

      1. hdfs-2117-1.patch
        0.4 kB
        Eli Collins

        Issue Links

          Activity

          Hide
          Eli Collins added a comment -

          Minimal patch attached.

          Show
          Eli Collins added a comment - Minimal patch attached.
          Hide
          Daryn Sharp added a comment -

          +1 looks good, shame that it's not an exception...

          Show
          Daryn Sharp added a comment - +1 looks good, shame that it's not an exception...
          Hide
          Suresh Srinivas added a comment -

          I am with Daryn here. API that return true/false and throw IOException are ugly. Should we change it to throw exception, instead of return false?

          Show
          Suresh Srinivas added a comment - I am with Daryn here. API that return true/false and throw IOException are ugly. Should we change it to throw exception, instead of return false?
          Hide
          Suresh Srinivas added a comment -

          Continuing my previous comment, should not return boolean at all from the method?

          Show
          Suresh Srinivas added a comment - Continuing my previous comment, should not return boolean at all from the method?
          Hide
          Eli Collins added a comment -

          I agree using exceptions would be better, for some reason exceptions were used in the trunk version of HADOOP-6566 but not in the YDH patch which introduced this code. How about we fix the current function (which should be returning false in this case) and file another jira to bring the code in line with trunk?

          Show
          Eli Collins added a comment - I agree using exceptions would be better, for some reason exceptions were used in the trunk version of HADOOP-6566 but not in the YDH patch which introduced this code. How about we fix the current function (which should be returning false in this case) and file another jira to bring the code in line with trunk?
          Hide
          Suresh Srinivas added a comment -

          Given that this is a minor change - why not just do in this?

          Show
          Suresh Srinivas added a comment - Given that this is a minor change - why not just do in this?
          Hide
          Eli Collins added a comment -

          Are you referring to throwing an exception here instead of returning false? Given the rest of the method returns true/false I think we should be consistent. I do think it's also reasonable to replace the entire function and it's caller(s) with the code from trunk, however that seems like it can be done separately. Presumably Arun generated different patches for 20/trunk for a reason, though I haven't looked closely.

          Show
          Eli Collins added a comment - Are you referring to throwing an exception here instead of returning false? Given the rest of the method returns true/false I think we should be consistent. I do think it's also reasonable to replace the entire function and it's caller(s) with the code from trunk, however that seems like it can be done separately. Presumably Arun generated different patches for 20/trunk for a reason, though I haven't looked closely.
          Hide
          Suresh Srinivas added a comment -

          I think this is not a big deal... +1 for the change.

          Show
          Suresh Srinivas added a comment - I think this is not a big deal... +1 for the change.
          Hide
          Eli Collins added a comment -

          Thanks Suresh. I've committed this.

          Show
          Eli Collins added a comment - Thanks Suresh. I've committed this.
          Hide
          Matt Foley added a comment -

          Closed upon release of 0.20.205.0

          Show
          Matt Foley added a comment - Closed upon release of 0.20.205.0

            People

            • Assignee:
              Eli Collins
              Reporter:
              Eli Collins
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development