HBase
  1. HBase
  2. HBASE-7465

CleanerChore shouldn't leak IOException for failed delete of non-empty directory

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Duplicate
    • Affects Version/s: 0.94.4, 0.95.2
    • Fix Version/s: None
    • Component/s: master
    • Labels:
      None

      Description

      CleanerChore has the following lines:

          // if all the children have been deleted, then we should try to delete this directory. However,
          // don't do so recursively so we don't delete files that have been added since we checked.
          return canDeleteThis ? fs.delete(toCheck, false) : false;
      

      has a race condition where the directory can have a file added underneath of it between being determined 'ok to delete' and not. As the comment mentions, we don't do a recursive delete to avoid deleting those files. However, the non-recursive delete of the directory throws an IOException.

      This is from http://search-hadoop.com/m/MUMTb13obDf2/CleanerChore+exception/v=plain

        Activity

        Hide
        Lars Hofhansl added a comment -

        HBASE-7467 seems to describe a slightly different scenario.

        Show
        Lars Hofhansl added a comment - HBASE-7467 seems to describe a slightly different scenario.
        Hide
        Jesse Yates added a comment -

        It is indeed, but I don't think its actually an issue...contining comments there.

        Show
        Jesse Yates added a comment - It is indeed, but I don't think its actually an issue...contining comments there.
        Hide
        Jesse Yates added a comment -

        Attaching patch. Makes CleanerChore#checkAndDeleteDirectory default scoped and adds a test to make sure we don't throw an exception even when we have the 'racey' file condition.

        Added test and existing TestCleanerChore tests pass locally.

        Show
        Jesse Yates added a comment - Attaching patch. Makes CleanerChore#checkAndDeleteDirectory default scoped and adds a test to make sure we don't throw an exception even when we have the 'racey' file condition. Added test and existing TestCleanerChore tests pass locally.
        Hide
        Jean-Marc Spaggiari added a comment -

        UTIL.cleanupTestDir(); is not compiling for me Is it for trunk only? I tried to apply on 0.94.4RC0.

        Show
        Jean-Marc Spaggiari added a comment - UTIL.cleanupTestDir(); is not compiling for me Is it for trunk only? I tried to apply on 0.94.4RC0.
        Hide
        Jean-Marc Spaggiari added a comment -

        Ok. I replaced UTIL.cleanupTestDir() by what was there before and everything is working A1 for me. All tests from TestCleanerChore passed.

        So if the UTIL "issue" I have is because I'm not running on trunk, then I will say +1 for me.

        Show
        Jean-Marc Spaggiari added a comment - Ok. I replaced UTIL.cleanupTestDir() by what was there before and everything is working A1 for me. All tests from TestCleanerChore passed. So if the UTIL "issue" I have is because I'm not running on trunk, then I will say +1 for me.
        Hide
        Lars Hofhansl added a comment -

        Looks good to me +1.
        Wanna make the log message debug?

        Show
        Lars Hofhansl added a comment - Looks good to me +1. Wanna make the log message debug?
        Hide
        Jesse Yates added a comment -

        This work got picked up in HBASE-7467, so just duping it and moving on.

        Show
        Jesse Yates added a comment - This work got picked up in HBASE-7467 , so just duping it and moving on.

          People

          • Assignee:
            Jesse Yates
            Reporter:
            Jesse Yates
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development