Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-1149

Lease reassignment is not persisted to edit log

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.21.0, 0.22.0, 0.23.0
    • Fix Version/s: 0.23.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      During lease recovery, the lease gets reassigned to a special NN holder. This is not currently persisted to the edit log, which means that after an NN restart, the original leaseholder could end up allocating more blocks or completing a file that has already started recovery.

      1. hdfs-1149.0.patch
        20 kB
        Aaron T. Myers
      2. hdfs-1149.1.patch
        30 kB
        Aaron T. Myers
      3. editsStored
        2 kB
        Aaron T. Myers
      4. hdfs-1149.1.patch
        30 kB
        Aaron T. Myers
      5. hdfs-1149.2.patch
        30 kB
        Aaron T. Myers

        Issue Links

          Activity

          Todd Lipcon created issue -
          Todd Lipcon made changes -
          Field Original Value New Value
          Link This issue is related to HDFS-1142 [ HDFS-1142 ]
          Hide
          sam rash added a comment -

          Hey Todd,

          Can you please specify the sequence of events that occur and the ending error state?

          Show
          sam rash added a comment - Hey Todd, Can you please specify the sequence of events that occur and the ending error state?
          Hide
          Todd Lipcon added a comment -

          This also affects HA failover.

          Show
          Todd Lipcon added a comment - This also affects HA failover.
          Todd Lipcon made changes -
          Link This issue relates to HDFS-1623 [ HDFS-1623 ]
          Aaron T. Myers made changes -
          Assignee Aaron T. Myers [ atm ]
          Hide
          Aaron T. Myers added a comment -

          Patch which addresses the issue.

          I changed around the waitActive method of MiniDFSCluster such that it will work both on fresh NN starts and NN restarts. This consisted of moving some error handling code around the call to waitActive from restartNameNode into waitActive itself.

          Show
          Aaron T. Myers added a comment - Patch which addresses the issue. I changed around the waitActive method of MiniDFSCluster such that it will work both on fresh NN starts and NN restarts. This consisted of moving some error handling code around the call to waitActive from restartNameNode into waitActive itself.
          Aaron T. Myers made changes -
          Attachment hdfs-1149.0.patch [ 12481177 ]
          Aaron T. Myers made changes -
          Fix Version/s 0.23.0 [ 12315571 ]
          Affects Version/s 0.22.0 [ 12314241 ]
          Affects Version/s 0.23.0 [ 12315571 ]
          Aaron T. Myers made changes -
          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/12481177/hdfs-1149.0.patch
          against trunk revision 1130339.

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

          +1 tests included. The patch appears to include 6 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 (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.hdfs.TestDFSUpgradeFromImage
          org.apache.hadoop.hdfs.TestHDFSTrash
          org.apache.hadoop.hdfs.TestHFlush
          org.apache.hadoop.hdfs.tools.offlineEditsViewer.TestOfflineEditsViewer
          org.apache.hadoop.hdfs.tools.offlineImageViewer.TestOfflineImageViewer

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/680//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/680//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/680//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/12481177/hdfs-1149.0.patch against trunk revision 1130339. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.TestDFSUpgradeFromImage org.apache.hadoop.hdfs.TestHDFSTrash org.apache.hadoop.hdfs.TestHFlush org.apache.hadoop.hdfs.tools.offlineEditsViewer.TestOfflineEditsViewer org.apache.hadoop.hdfs.tools.offlineImageViewer.TestOfflineImageViewer +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/680//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/680//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/680//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          A few nits:

          • for DataNode.setHeartbeatsEnabled, I think it would be better to make it package-private, and then bounce through the "DataNodeAdapter" class to get at it. I also think it would be clearer if we inverted its meaning and renamed it to heartbeatsDisabledForTests - that way when reading the code later it will be clear that this is always false in normal operation.
          • Same goes for all of the new public members in LeaseManager/Lease – I think you can just move the getLeaseByPath function into NameNodeAdapter, then it can all stay package-protected, right?
          • In the test case, I think it's better to call stm.hflush() after the writer has lost its lease – this is a DN-only operation, which means that it's verifying that the lease recovery has gone all the way through, not just a NN state change. The fact that you check isUnderConstruction should already do that as well, but just a double-check. Then you can close the stream as well and check for the same exception.
          • I think the new NAMENODE_LEASE_MANAGER_SLEEP_TIME is probably better named NAMENODE_LEASE_RECHECK_INTERVAL (more consistent with other variables like heartbeatRecheckInterval and replicationRecheckInterval)

          Other concern:

          • Does this interact correctly with lease maintenance on rename/delete? I think so... but it would be good to add the following tests:

          Test A:
          1) client creates file /dir_a/file and leaves it open
          2) client renames /dir_a to /dir_b (this calls LeaseManager.changeLease)
          3) client dies, so lease recovery happens
          4) NN reassigns lease to NN_Recovery
          5) NN restarts and loads edits: NN_Recovery should own the lease on the new location of the file

          [ this tests that on edit log replay, the lease is properly tracked to the new name of the file ]

          Test B:
          1) client creates file /file and leaves it open
          2) client deletes file /file
          3) client dies, so lease recovery happens
          4) NN reassigns lease to NN_Recovery
          5) NN restarts and loads edits: no NPEs or anything

          I'm also wondering if we have an issue with regards to safeMode. In theory we should never write anything to the edit log while in safemode, but I don't see safemode checks in internalReleaseLease. This is similar to the bugs seen in HDFS-988 if you want some background

          Show
          Todd Lipcon added a comment - A few nits: for DataNode.setHeartbeatsEnabled, I think it would be better to make it package-private, and then bounce through the "DataNodeAdapter" class to get at it. I also think it would be clearer if we inverted its meaning and renamed it to heartbeatsDisabledForTests - that way when reading the code later it will be clear that this is always false in normal operation. Same goes for all of the new public members in LeaseManager/Lease – I think you can just move the getLeaseByPath function into NameNodeAdapter, then it can all stay package-protected, right? In the test case, I think it's better to call stm.hflush() after the writer has lost its lease – this is a DN-only operation, which means that it's verifying that the lease recovery has gone all the way through, not just a NN state change. The fact that you check isUnderConstruction should already do that as well, but just a double-check. Then you can close the stream as well and check for the same exception. I think the new NAMENODE_LEASE_MANAGER_SLEEP_TIME is probably better named NAMENODE_LEASE_RECHECK_INTERVAL (more consistent with other variables like heartbeatRecheckInterval and replicationRecheckInterval ) Other concern: Does this interact correctly with lease maintenance on rename/delete? I think so... but it would be good to add the following tests: Test A: 1) client creates file /dir_a/file and leaves it open 2) client renames /dir_a to /dir_b (this calls LeaseManager.changeLease) 3) client dies, so lease recovery happens 4) NN reassigns lease to NN_Recovery 5) NN restarts and loads edits: NN_Recovery should own the lease on the new location of the file [ this tests that on edit log replay, the lease is properly tracked to the new name of the file ] Test B: 1) client creates file /file and leaves it open 2) client deletes file /file 3) client dies, so lease recovery happens 4) NN reassigns lease to NN_Recovery 5) NN restarts and loads edits: no NPEs or anything I'm also wondering if we have an issue with regards to safeMode. In theory we should never write anything to the edit log while in safemode, but I don't see safemode checks in internalReleaseLease. This is similar to the bugs seen in HDFS-988 if you want some background
          Hide
          Aaron T. Myers added a comment -

          Thanks a lot for the review, Todd.

          Attached is an updated patch addressing Todd's comments.

          I didn't add a test case for case "B", where the client deletes the file and then lease recovery occurs, because this can't actually happen. If the client deletes the file before losing its lease, the lease is removed from the set of leases the NN is keeping track of, so lease recovery will never occur.

          Note that TestOfflineEditsViewer will fail unless the editsStored file attached to this JIRA is placed in src/test/hdfs/org/apache/hadoop/hdfs/tools/offlineEditsViewer/editsStored.

          Show
          Aaron T. Myers added a comment - Thanks a lot for the review, Todd. Attached is an updated patch addressing Todd's comments. I didn't add a test case for case "B", where the client deletes the file and then lease recovery occurs, because this can't actually happen. If the client deletes the file before losing its lease, the lease is removed from the set of leases the NN is keeping track of, so lease recovery will never occur. Note that TestOfflineEditsViewer will fail unless the editsStored file attached to this JIRA is placed in src/test/hdfs/org/apache/hadoop/hdfs/tools/offlineEditsViewer/editsStored .
          Aaron T. Myers made changes -
          Attachment hdfs-1149.1.patch [ 12481595 ]
          Attachment editsStored [ 12481596 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12481596/editsStored
          against trunk revision 1132715.

          +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 new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

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

          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/721//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/12481596/editsStored against trunk revision 1132715. +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 new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/721//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          Resubmitting the patch since test-patch tried to apply the binary editsStored file.

          Show
          Aaron T. Myers added a comment - Resubmitting the patch since test-patch tried to apply the binary editsStored file.
          Aaron T. Myers made changes -
          Attachment hdfs-1149.1.patch [ 12481598 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12481598/hdfs-1149.1.patch
          against trunk revision 1132715.

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

          +1 tests included. The patch appears to include 20 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 (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.hdfs.tools.offlineEditsViewer.TestOfflineEditsViewer

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/722//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/722//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/722//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/12481598/hdfs-1149.1.patch against trunk revision 1132715. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 20 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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.tools.offlineEditsViewer.TestOfflineEditsViewer +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/722//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/722//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/722//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          It seems to me that the new condition for safe mode check in internalReleaseLease should actually be an assert, right?
          The two call paths into internalReleaseLease are:

          • FSNamesystem.startFileInternal, which checks safeMode at the top
          • LeaseManager.checkLeases(), which is called from LeaseManager.Monitor inside a check against safemode

          so, any call of internalReleaseLease not in safe mode would be a real bug, right?

          Otherwise looks good.

          Show
          Todd Lipcon added a comment - It seems to me that the new condition for safe mode check in internalReleaseLease should actually be an assert, right? The two call paths into internalReleaseLease are: FSNamesystem.startFileInternal, which checks safeMode at the top LeaseManager.checkLeases(), which is called from LeaseManager.Monitor inside a check against safemode so, any call of internalReleaseLease not in safe mode would be a real bug, right? Otherwise looks good.
          Hide
          Aaron T. Myers added a comment -

          Thanks a lot for the review, Todd. The only change in this latest patch is:

          -    if (isInSafeMode())
          -      throw new SafeModeException("Cannot release lease on " + src, safeMode);
          +    assert !isInSafeMode();
          
          Show
          Aaron T. Myers added a comment - Thanks a lot for the review, Todd. The only change in this latest patch is: - if (isInSafeMode()) - throw new SafeModeException("Cannot release lease on " + src, safeMode); + assert !isInSafeMode();
          Aaron T. Myers made changes -
          Attachment hdfs-1149.2.patch [ 12481613 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12481613/hdfs-1149.2.patch
          against trunk revision 1132715.

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

          +1 tests included. The patch appears to include 20 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 (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.hdfs.server.balancer.TestBalancer
          org.apache.hadoop.hdfs.tools.offlineEditsViewer.TestOfflineEditsViewer

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/724//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/724//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/724//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/12481613/hdfs-1149.2.patch against trunk revision 1132715. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 20 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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.server.balancer.TestBalancer org.apache.hadoop.hdfs.tools.offlineEditsViewer.TestOfflineEditsViewer +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/724//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/724//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/724//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          TestBalancer just passed for me on my local box.

          Show
          Aaron T. Myers added a comment - TestBalancer just passed for me on my local box.
          Hide
          Todd Lipcon added a comment -

          +1. Committed just to trunk, even though this affects all past versions. Since it requires an edits log format change, etc, I figured it's better to just do 0.23. If we run into this we can backport some other day.

          Show
          Todd Lipcon added a comment - +1. Committed just to trunk, even though this affects all past versions. Since it requires an edits log format change, etc, I figured it's better to just do 0.23. If we run into this we can backport some other day.
          Todd Lipcon made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #746 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/746/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #746 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/746/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #699 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/699/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #699 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/699/ )
          Hide
          Tsz Wo Nicholas Sze added a comment -

          DFSClientAdapter should be put under test; filed HDFS-2153.

          Show
          Tsz Wo Nicholas Sze added a comment - DFSClientAdapter should be put under test; filed HDFS-2153 .
          Tsz Wo Nicholas Sze made changes -
          Link This issue relates to HDFS-3181 [ HDFS-3181 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          385d 4h 45m 1 Aaron T. Myers 02/Jun/11 01:41
          Patch Available Patch Available Resolved Resolved
          4d 21h 43m 1 Todd Lipcon 06/Jun/11 23:25

            People

            • Assignee:
              Aaron T. Myers
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development