Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-3936

MiniDFSCluster shutdown races with BlocksMap usage

    Details

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

      Description

      Looks like HDFS-3664 didn't fix the whole issue because the added join times out because the thread closing the BM (FSN#stopCommonServices) holds the FSN lock while closing the BM and the BM is block uninterruptedly trying to aquire the FSN lock.

      2012-09-13 18:54:12,526 FATAL hdfs.MiniDFSCluster (MiniDFSCluster.java:shutdown(1355)) - Test resulted in an unexpected exit
      org.apache.hadoop.util.ExitUtil$ExitException: Fatal exception with message null
      stack trace
      java.lang.NullPointerException
      	at org.apache.hadoop.hdfs.server.blockmanagement.BlocksMap.getBlockCollection(BlocksMap.java:101)
      	at org.apache.hadoop.hdfs.server.blockmanagement.BlockManager.computeReplicationWorkForBlocks(BlockManager.java:1132)
      	at org.apache.hadoop.hdfs.server.blockmanagement.BlockManager.computeReplicationWork(BlockManager.java:1107)
      	at org.apache.hadoop.hdfs.server.blockmanagement.BlockManager.computeDatanodeWork(BlockManager.java:3061)
      	at org.apache.hadoop.hdfs.server.blockmanagement.BlockManager$ReplicationMonitor.run(BlockManager.java:3023)
      	at java.lang.Thread.run(Thread.java:662)
      
      1. hdfs-3936.txt
        14 kB
        Eli Collins
      2. hdfs-3936.txt
        0.9 kB
        Eli Collins
      3. hdfs-3936.txt
        2 kB
        Eli Collins

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1201 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1201/)
          HDFS-3936. MiniDFSCluster shutdown races with BlocksMap usage. Contributed by Eli Collins (Revision 1387255)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1387255
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlocksMap.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1201 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1201/ ) HDFS-3936 . MiniDFSCluster shutdown races with BlocksMap usage. Contributed by Eli Collins (Revision 1387255) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1387255 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlocksMap.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1170 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1170/)
          HDFS-3936. MiniDFSCluster shutdown races with BlocksMap usage. Contributed by Eli Collins (Revision 1387255)

          Result = FAILURE
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1387255
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlocksMap.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1170 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1170/ ) HDFS-3936 . MiniDFSCluster shutdown races with BlocksMap usage. Contributed by Eli Collins (Revision 1387255) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1387255 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlocksMap.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2765 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2765/)
          HDFS-3936. MiniDFSCluster shutdown races with BlocksMap usage. Contributed by Eli Collins (Revision 1387255)

          Result = FAILURE
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1387255
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlocksMap.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2765 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2765/ ) HDFS-3936 . MiniDFSCluster shutdown races with BlocksMap usage. Contributed by Eli Collins (Revision 1387255) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1387255 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlocksMap.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java
          Hide
          Eli Collins added a comment -

          I've committed this and merged to branch-2. Thanks for the reviews guys.

          Show
          Eli Collins added a comment - I've committed this and merged to branch-2. Thanks for the reviews guys.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2804 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2804/)
          HDFS-3936. MiniDFSCluster shutdown races with BlocksMap usage. Contributed by Eli Collins (Revision 1387255)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1387255
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlocksMap.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2804 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2804/ ) HDFS-3936 . MiniDFSCluster shutdown races with BlocksMap usage. Contributed by Eli Collins (Revision 1387255) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1387255 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlocksMap.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2741 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2741/)
          HDFS-3936. MiniDFSCluster shutdown races with BlocksMap usage. Contributed by Eli Collins (Revision 1387255)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1387255
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlocksMap.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2741 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2741/ ) HDFS-3936 . MiniDFSCluster shutdown races with BlocksMap usage. Contributed by Eli Collins (Revision 1387255) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1387255 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlocksMap.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java
          Hide
          Eli Collins added a comment -

          Looping this patch over night I ran into HDFS-3948, that's a separate issue.

          Show
          Eli Collins added a comment - Looping this patch over night I ran into HDFS-3948 , that's a separate issue.
          Hide
          Todd Lipcon added a comment -

          +1, if this gets our build back to green, I'm all for it!

          Show
          Todd Lipcon added a comment - +1, if this gets our build back to green, I'm all for it!
          Hide
          Eli Collins added a comment -

          But, but... I looked at LightWeightGSet.java and it did implement remove. I must be missing something... Also, don't have other problems if we don't support remove, like a memory leak?

          Note that I said the iterator doesn't support remove.

          Show
          Eli Collins added a comment - But, but... I looked at LightWeightGSet.java and it did implement remove. I must be missing something... Also, don't have other problems if we don't support remove, like a memory leak? Note that I said the iterator doesn't support remove.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestDatanodeBlockScanner

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3197//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3197//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/12545257/hdfs-3936.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +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 unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDatanodeBlockScanner +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3197//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3197//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3196//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3196//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/12545257/hdfs-3936.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +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 passed unit tests in hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3196//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3196//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          Looping and adding a timeout to the test revealed that BlocksMap#block's iterator doesn't support remove.

          But, but... I looked at LightWeightGSet.java and it did implement remove. I must be missing something... Also, don't have other problems if we don't support remove, like a memory leak?

          Show
          Colin Patrick McCabe added a comment - Looping and adding a timeout to the test revealed that BlocksMap#block's iterator doesn't support remove. But, but... I looked at LightWeightGSet.java and it did implement remove. I must be missing something... Also, don't have other problems if we don't support remove, like a memory leak?
          Hide
          Eli Collins added a comment -

          Updated patch attached.

          Show
          Eli Collins added a comment - Updated patch attached.
          Hide
          Eli Collins added a comment -

          Looping and adding a timeout to the test revealed that BlocksMap#block's iterator doesn't support remove. This indicates most executions of this test interrupt the BlockManager before it closes, in which case we should be able to just not null-out blocks. If there are OOM issues we can bump the memory usage or implement LightWeightGSet#clear. I'll file a jira for that.

          Show
          Eli Collins added a comment - Looping and adding a timeout to the test revealed that BlocksMap#block's iterator doesn't support remove. This indicates most executions of this test interrupt the BlockManager before it closes, in which case we should be able to just not null-out blocks. If there are OOM issues we can bump the memory usage or implement LightWeightGSet#clear. I'll file a jira for that.
          Hide
          Colin Patrick McCabe added a comment -

          Seems like a good stopgap to me, until we fix the locking issues. Looks good to me.

          Show
          Colin Patrick McCabe added a comment - Seems like a good stopgap to me, until we fix the locking issues. Looks good to me.
          Hide
          Todd Lipcon added a comment -

          Looks reasonable to me. I agree that this fix makes more sense for now - the patch that changed all the locks to interruptible made me nervous for some reason.

          Show
          Todd Lipcon added a comment - Looks reasonable to me. I agree that this fix makes more sense for now - the patch that changed all the locks to interruptible made me nervous for some reason.
          Hide
          Eli Collins added a comment -

          Patch attached. BlocksMap#close now empties the blocks map. Not a fix for the synchronization issues but fixes the frequent test failure and should also not have the OOM issues seen in HDFS-1114. I've been looping this on two machines and so far so good.

          Show
          Eli Collins added a comment - Patch attached. BlocksMap#close now empties the blocks map. Not a fix for the synchronization issues but fixes the frequent test failure and should also not have the OOM issues seen in HDFS-1114 . I've been looping this on two machines and so far so good.
          Hide
          Eli Collins added a comment -

          Colin,
          Good point - HDFS-1114 added it because some of the tests were failing due to OOM w/o it, however that's surprising given that the new structure uses less memory than the old one and we previously didn't null out the old one. In BlocksMap#close we could also just remove all the blocks rather than null out the variable.

          Show
          Eli Collins added a comment - Colin, Good point - HDFS-1114 added it because some of the tests were failing due to OOM w/o it, however that's surprising given that the new structure uses less memory than the old one and we previously didn't null out the old one. In BlocksMap#close we could also just remove all the blocks rather than null out the variable.
          Hide
          Colin Patrick McCabe added a comment -

          I swear I only submitted the preceding comment once. Looks like JIRA is using a relaxed consistency model today.

          Show
          Colin Patrick McCabe added a comment - I swear I only submitted the preceding comment once. Looks like JIRA is using a relaxed consistency model today.
          Hide
          Colin Patrick McCabe added a comment -

          If we're going for the absolute minimum change, perhaps we should just not set blocks = null in the close method? I'm not clear on why that is necessary in the first place (doesn't that whole object get GC'ed?) Haven't looked at its lifecycle closely though.

          Show
          Colin Patrick McCabe added a comment - If we're going for the absolute minimum change, perhaps we should just not set blocks = null in the close method? I'm not clear on why that is necessary in the first place (doesn't that whole object get GC'ed?) Haven't looked at its lifecycle closely though.
          Hide
          Colin Patrick McCabe added a comment -

          If we're going for the absolute minimum change, perhaps we should just not set blocks = null in the close method? I'm not clear on why that is necessary in the first place (doesn't that whole object get GC'ed?) Haven't looked at its lifecycle closely though.

          Show
          Colin Patrick McCabe added a comment - If we're going for the absolute minimum change, perhaps we should just not set blocks = null in the close method? I'm not clear on why that is necessary in the first place (doesn't that whole object get GC'ed?) Haven't looked at its lifecycle closely though.
          Hide
          Eli Collins added a comment -

          @Colin, the exception here is not unexpected, so asserting on IE here would mean shutdown fails.

          @Todd, BM#updatedNeededReplications is the only place this patch swallows the IOE, think think it should be propagated to all callers? The IE comes from the interrupt in BM#close which subsequently swallows the IE so it seemed equivalent. I could add Thread.currentThread().interrupt() so we throw an IE again but that will just get swallowed right?

          The top-level RPC methods and test util methods turn the IE into an IOE, think those should be preserved as IE as well? IIUC the RPC code will marshal it into an IOE anyway.

          While looping TestDFSClientRetries I found a related issue. Interrupting out of the RM lock fixes the issue where the BM does not actually exiting and races with the replication monitor (since it now gets interrupted), but client RPCs can still race with NN shutdown. After fixing this TestDFSClientRetries eventually fails with:

            Exception 0: org.apache.hadoop.ipc.RemoteException(org.apache.hadoop.ipc.RemoteException): java.lang.NullPointerException
                  at org.apache.hadoop.hdfs.server.blockmanagement.BlocksMap.getBlockCollection(BlocksMap.java:101)
                  at org.apache.hadoop.hdfs.server.blockmanagement.BlockManager.getBlockCollection(BlockManager.java:2947)
                  at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.isValidBlock(FSNamesystem.java:4477)
                  at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.allocateBlock(FSNamesystem.java:2460)
                  at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.getAdditionalBlock(FSNamesystem.java:2221)
                  at org.apache.hadoop.hdfs.server.namenode.NameNodeRpcServer.addBlock(NameNodeRpcServer.java:476)
          

          The NN should really stop the RPC server and drain all RPCs before shutting down the FSN, BM etc. Thinking that should be punted to another change. With the following this test passes when looped for 10 hours because this test only races on NN#addBlock.

          +  private boolean isClosed() {
          +    return blocks == null;
          +  }
          +
             BlockCollection getBlockCollection(Block b) {
          +    if (isClosed()) {
          +      return null; // This call raced with close
          +    }
          
          Show
          Eli Collins added a comment - @Colin, the exception here is not unexpected, so asserting on IE here would mean shutdown fails. @Todd, BM#updatedNeededReplications is the only place this patch swallows the IOE, think think it should be propagated to all callers? The IE comes from the interrupt in BM#close which subsequently swallows the IE so it seemed equivalent. I could add Thread.currentThread().interrupt() so we throw an IE again but that will just get swallowed right? The top-level RPC methods and test util methods turn the IE into an IOE, think those should be preserved as IE as well? IIUC the RPC code will marshal it into an IOE anyway. While looping TestDFSClientRetries I found a related issue. Interrupting out of the RM lock fixes the issue where the BM does not actually exiting and races with the replication monitor (since it now gets interrupted), but client RPCs can still race with NN shutdown. After fixing this TestDFSClientRetries eventually fails with: Exception 0: org.apache.hadoop.ipc.RemoteException(org.apache.hadoop.ipc.RemoteException): java.lang.NullPointerException at org.apache.hadoop.hdfs.server.blockmanagement.BlocksMap.getBlockCollection(BlocksMap.java:101) at org.apache.hadoop.hdfs.server.blockmanagement.BlockManager.getBlockCollection(BlockManager.java:2947) at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.isValidBlock(FSNamesystem.java:4477) at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.allocateBlock(FSNamesystem.java:2460) at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.getAdditionalBlock(FSNamesystem.java:2221) at org.apache.hadoop.hdfs.server.namenode.NameNodeRpcServer.addBlock(NameNodeRpcServer.java:476) The NN should really stop the RPC server and drain all RPCs before shutting down the FSN, BM etc. Thinking that should be punted to another change. With the following this test passes when looped for 10 hours because this test only races on NN#addBlock. + private boolean isClosed() { + return blocks == null ; + } + BlockCollection getBlockCollection(Block b) { + if (isClosed()) { + return null ; // This call raced with close + }
          Hide
          Todd Lipcon added a comment -

          Haven't looked in detail at this yet, but I noticed several cases that interruptions are caught. In general in these cases it's better to also do a Thread.currentThread().interrupt() so that the interrupt status is maintained, so that the next sleep() or interruptible action will also get interrupted, until eventually somebody handles the status.

          Show
          Todd Lipcon added a comment - Haven't looked in detail at this yet, but I noticed several cases that interruptions are caught. In general in these cases it's better to also do a Thread.currentThread().interrupt() so that the interrupt status is maintained, so that the next sleep() or interruptible action will also get interrupted, until eventually somebody handles the status.
          Hide
          Colin Patrick McCabe added a comment -
             private void updateNeededReplications(final Block block,
                 final int curReplicasDelta, int expectedReplicasDelta) {
          -    namesystem.writeLock();
          +    try {
          +      updateNeededReplicationsInterruptible(
          +          block, curReplicasDelta, expectedReplicasDelta);
          +    } catch (InterruptedException ie) {
          +      LOG.warn("Interrupted while updating replication queues");
          +    }
          +  }
          

          I don't like this function because it swallows exceptions. If an unexpected exception happened, we shouldn't swallow it-- we should assert.

          Alternately, perhaps updateNeededReplications should take a boolean that specifies whether it locks interruptibly. This probably would be the simplest way out of the interruptibility dilemma.

          Show
          Colin Patrick McCabe added a comment - private void updateNeededReplications( final Block block, final int curReplicasDelta, int expectedReplicasDelta) { - namesystem.writeLock(); + try { + updateNeededReplicationsInterruptible( + block, curReplicasDelta, expectedReplicasDelta); + } catch (InterruptedException ie) { + LOG.warn( "Interrupted while updating replication queues" ); + } + } I don't like this function because it swallows exceptions. If an unexpected exception happened, we shouldn't swallow it-- we should assert. Alternately, perhaps updateNeededReplications should take a boolean that specifies whether it locks interruptibly. This probably would be the simplest way out of the interruptibility dilemma.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestDatanodeBlockScanner

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3191//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3191//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/12545118/hdfs-3936.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +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 unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDatanodeBlockScanner +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3191//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3191//console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          Patch attached. I'm looping TestWebHDFS with this which reliably reproduces the issue.

          Show
          Eli Collins added a comment - Patch attached. I'm looping TestWebHDFS with this which reliably reproduces the issue.
          Hide
          Eli Collins added a comment -

          I'll see what the patch for #3 looks like, it was my first instinct but thought it a little goofy to eg fail an addBlock RPC because the replication monitor is interrupted.

          The replication manager does a lot of stuff, and it really seems like we're asking for trouble if we don't shut it down at the end.

          With #4 the replication manager is shutdown cleanly, it unwinds because all replication queues and the block map are shutdown. Ie rather than NPE in this case it finishes it's current cycle and bails cleanly. The only thing that BM#close does that prevents this is null out the array in BlocksMap.

          Show
          Eli Collins added a comment - I'll see what the patch for #3 looks like, it was my first instinct but thought it a little goofy to eg fail an addBlock RPC because the replication monitor is interrupted. The replication manager does a lot of stuff, and it really seems like we're asking for trouble if we don't shut it down at the end. With #4 the replication manager is shutdown cleanly, it unwinds because all replication queues and the block map are shutdown. Ie rather than NPE in this case it finishes it's current cycle and bails cleanly. The only thing that BM#close does that prevents this is null out the array in BlocksMap.
          Hide
          Colin Patrick McCabe added a comment -

          I would lean towards solution #3. It might need a little bit of finesse, but it should be simple in theory to have the lock semantic of "wait for us to get the lock or be told to exit."

          I'm afraid of hitting other issues if we go with #4, since BlockManager#replicationThread touches a lot more stuff than just BlocksMap. The replication manager does a lot of stuff, and it really seems like we're asking for trouble if we don't shut it down at the end.

          Show
          Colin Patrick McCabe added a comment - I would lean towards solution #3. It might need a little bit of finesse, but it should be simple in theory to have the lock semantic of "wait for us to get the lock or be told to exit." I'm afraid of hitting other issues if we go with #4, since BlockManager#replicationThread touches a lot more stuff than just BlocksMap. The replication manager does a lot of stuff, and it really seems like we're asking for trouble if we don't shut it down at the end.
          Hide
          Eli Collins added a comment -

          Options:

          1. Decouple BM and FSN locking (HDFS-3937). Overkill for this issue.
          2. Have the FSN quiesce the BM before it aquires the lock and closes the BM (so the BW won't attempt acquire the FSN lock)
          3. Have the BM try to aquire the FSN lock interruptedly
          4. Modify BM to cleanly deal with a closed blocksMap

          I think option #1 is the right (TM) answer but better done as part of HDFS-2184, so #4 seems simplest to fix this race.

          Show
          Eli Collins added a comment - Options: Decouple BM and FSN locking ( HDFS-3937 ). Overkill for this issue. Have the FSN quiesce the BM before it aquires the lock and closes the BM (so the BW won't attempt acquire the FSN lock) Have the BM try to aquire the FSN lock interruptedly Modify BM to cleanly deal with a closed blocksMap I think option #1 is the right (TM) answer but better done as part of HDFS-2184 , so #4 seems simplest to fix this race.
          Hide
          Colin Patrick McCabe added a comment -

          Hmm, what's the rationale for blocking uninterruptedly in the BM?

          Show
          Colin Patrick McCabe added a comment - Hmm, what's the rationale for blocking uninterruptedly in the BM?

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development