Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-1141

completeFile does not check lease ownership

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.20-append
    • Fix Version/s: 0.20-append, 0.20.205.0, 0.22.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      completeFile should check that the caller still owns the lease of the file that it's completing. This is for the 'testCompleteOtherLeaseHoldersFile' case in HDFS-1139.

      1. hdfs-1141-branch20.txt
        5 kB
        Todd Lipcon
      2. hdfs-1141.txt
        14 kB
        Todd Lipcon
      3. hdfs-1141.txt
        17 kB
        Todd Lipcon
      4. HDFS-1141.20-security.1.patch
        5 kB
        Jitendra Nath Pandey

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          Here's a patch with just those test cases that apply to this bug from HDFS-1139.

          Note: this does change the exception text coming out of completeFile to be more specific - now it actually contains "Lease mismatch" or "File is closed", etc, rather then the generic "Could not complete file" that it used to. I think this is a good thing, just wanted to note that it does have user-facing implications.

          Show
          Todd Lipcon added a comment - Here's a patch with just those test cases that apply to this bug from HDFS-1139 . Note: this does change the exception text coming out of completeFile to be more specific - now it actually contains "Lease mismatch" or "File is closed", etc, rather then the generic "Could not complete file" that it used to. I think this is a good thing, just wanted to note that it does have user-facing implications.
          Hide
          Todd Lipcon added a comment -

          Branch 20 patch for the HDFS-142-based append (not for commit to branch 20)

          Show
          Todd Lipcon added a comment - Branch 20 patch for the HDFS-142 -based append (not for commit to branch 20)
          Hide
          dhruba borthakur added a comment -

          I think the previous code clearly distinguished (via the log message) whether the file was a valid file that was already completed or whether it has a lease. Does the new log message work if the file is already closed?

          Show
          dhruba borthakur added a comment - I think the previous code clearly distinguished (via the log message) whether the file was a valid file that was already completed or whether it has a lease. Does the new log message work if the file is already closed?
          Hide
          Todd Lipcon added a comment -

          Yes, the LeaseExpiredException thrown by checkLease is informative - see FSNamesystem.checkLease. It will say "File does not exist" vs "File is not open for writing" vs "Lease mismatch" depending on the circumstance. This will make it to the log as well as back to the client (the prior implementation just gave back a generic IOE to the client saying it could not complete)

          Show
          Todd Lipcon added a comment - Yes, the LeaseExpiredException thrown by checkLease is informative - see FSNamesystem.checkLease. It will say "File does not exist" vs "File is not open for writing" vs "Lease mismatch" depending on the circumstance. This will make it to the log as well as back to the client (the prior implementation just gave back a generic IOE to the client saying it could not complete)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12444161/hdfs-1141.txt
          against trunk revision 942863.

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

          +1 tests included. The patch appears to include 7 new or modified tests.

          +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 failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/351/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/351/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/351/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/351/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/12444161/hdfs-1141.txt against trunk revision 942863. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 new or modified tests. +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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/351/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/351/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/351/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/351/console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Test failure is the normal "port out of range: -1" shenanigans, nothing related to this patch.

          Show
          Todd Lipcon added a comment - Test failure is the normal "port out of range: -1" shenanigans, nothing related to this patch.
          Hide
          dhruba borthakur added a comment -

          +1, code looks good. I will commit in a day unless somebody else has any other review comments.

          Show
          dhruba borthakur added a comment - +1, code looks good. I will commit in a day unless somebody else has any other review comments.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 thanks for working on it, Todd.

          Minor: Look like that pendingFile won't be null since the checkLease(..) method below will throw a LeaseExpiredException when file == null.

          //FSNamesystem
            private void checkLease(String src, String holder, INode file) 
                                                               throws IOException {
          
              if (file == null || file.isDirectory()) {
                Lease lease = leaseManager.getLease(holder);
                throw new LeaseExpiredException("No lease on " + src +
                ...
          
          Show
          Tsz Wo Nicholas Sze added a comment - +1 thanks for working on it, Todd. Minor: Look like that pendingFile won't be null since the checkLease(..) method below will throw a LeaseExpiredException when file == null. //FSNamesystem private void checkLease( String src, String holder, INode file) throws IOException { if (file == null || file.isDirectory()) { Lease lease = leaseManager.getLease(holder); throw new LeaseExpiredException( "No lease on " + src + ...
          Hide
          Todd Lipcon added a comment -

          Thanks for taking a look, Nicolas.

          Minor: Look like that pendingFile won't be null since the checkLease(..) method below will throw a LeaseExpiredException when file == null.

          Not sure what you're referring to - the patch removes the checks against pendingFile == null. We now check only for fileBlocks == null. Are you suggesting I remove fileBlocks and the associated null check? That seems reasonable, since fileBlocks itself isn't used outside the null check.

          Show
          Todd Lipcon added a comment - Thanks for taking a look, Nicolas. Minor: Look like that pendingFile won't be null since the checkLease(..) method below will throw a LeaseExpiredException when file == null. Not sure what you're referring to - the patch removes the checks against pendingFile == null. We now check only for fileBlocks == null. Are you suggesting I remove fileBlocks and the associated null check? That seems reasonable, since fileBlocks itself isn't used outside the null check.
          Hide
          Todd Lipcon added a comment -

          Updated patch based on Nicholas's feedback. I also removed CompleteFileStatus entirely, since we were only using the success/wait codes, it's easier to just use boolean.

          Show
          Todd Lipcon added a comment - Updated patch based on Nicholas's feedback. I also removed CompleteFileStatus entirely, since we were only using the success/wait codes, it's easier to just use boolean.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > Not sure what you're referring to ...
          Oops, I mixed up pendingFile and fileBlocks.

          I just have checked the codes. fileBlocks also won't possibly be null, given that pendingFile is an INodeFileUnderConstruction.

          +1 the new patch looks good.

          Show
          Tsz Wo Nicholas Sze added a comment - > Not sure what you're referring to ... Oops, I mixed up pendingFile and fileBlocks. I just have checked the codes. fileBlocks also won't possibly be null, given that pendingFile is an INodeFileUnderConstruction. +1 the new patch looks good.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12444208/hdfs-1141.txt
          against trunk revision 942863.

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

          +1 tests included. The patch appears to include 7 new or modified tests.

          +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 failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/354/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/354/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/354/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/354/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/12444208/hdfs-1141.txt against trunk revision 942863. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 new or modified tests. +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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/354/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/354/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/354/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/354/console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          The failed tests are unrelated (TestNodeCounts passes locally). I'll file a JIRA for the TestNodeCounts failure separately if there isn't already one, since it's rather worrisome!

          Show
          Todd Lipcon added a comment - The failed tests are unrelated (TestNodeCounts passes locally). I'll file a JIRA for the TestNodeCounts failure separately if there isn't already one, since it's rather worrisome!
          Hide
          dhruba borthakur added a comment -

          I just committed this. Thanks Todd!

          Show
          dhruba borthakur added a comment - I just committed this. Thanks Todd!
          Hide
          Todd Lipcon added a comment -

          This was determined to be invalid in trunk, but is necessary in branch-0.20-append

          Show
          Todd Lipcon added a comment - This was determined to be invalid in trunk, but is necessary in branch-0.20-append
          Hide
          Todd Lipcon added a comment -

          Oh wait, not unnecesary in trunk. But also necessary in branch-0.20 append I was thinking of that other similar JIRA about leases. Reopening for commit to 0.20-append

          Show
          Todd Lipcon added a comment - Oh wait, not unnecesary in trunk. But also necessary in branch-0.20 append I was thinking of that other similar JIRA about leases. Reopening for commit to 0.20-append
          Hide
          dhruba borthakur added a comment -

          Pulled into hadoop-0.20-append

          Show
          dhruba borthakur added a comment - Pulled into hadoop-0.20-append
          Hide
          Jitendra Nath Pandey added a comment -

          Patch for 20-security uploaded.

          Show
          Jitendra Nath Pandey added a comment - Patch for 20-security uploaded.
          Hide
          Suresh Srinivas added a comment -

          +1 for the patch. I committed it to 0.20-security branch.

          Show
          Suresh Srinivas added a comment - +1 for the patch. I committed it to 0.20-security branch.
          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:
              Todd Lipcon
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development