Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-1955

FSImage.doUpgrade() was made too fault-tolerant by HDFS-1826

    Details

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

      Description

      Prior to HDFS-1826, doUpgrade() would fail if any of the storage directories failed to successfully write the new fsimage or edits files.
      Now it appears to "succeed" even if some or all of the individual directories fail.

      There is some discussion about whether doUpgrade() should have some fault tolerance, but for now make it fail on any single storage directory failure, as before.

      1. hdfs-1955_1.patch
        8 kB
        Matt Foley
      2. hdfs-1955_1.patch
        8 kB
        Matt Foley
      3. hdfs-1955_2.patch
        8 kB
        Matt Foley

        Issue Links

          Activity

          Hide
          Matt Foley added a comment -

          Here is a patch that provides the desired check, failing doUpgrade() if any storage directory fails. The change in FSImage is just a few lines, and easily validated by inspection.

          However, providing a unit test for it was very difficult. The problem is that failure must be forced within the doUpgrade() method itself, which is buried in the Namenode startup code, and quite well protected. First I tried to make the storage dir read-only, but that gets caught in recoverTransitionRead() well before invoking doUpgrade(). Second I looked at using Mockito, but it seems that in order to spy on the startup/upgrade process one would have to mock the entire stack of HDFS system objects. The invocation of NNStorage.rename() at line 367 of FSImage would be a convenient spy target, but it is static and I saw no way to get hold of it. Third, I rejected non-mock test parameters in production code.

          Finally I just tested it manually by temporarily hacking the code in doUpgrade() to force the error. I was able to validate my patch, and also found and fixed an NPE bug in FSEditLog.

          Show
          Matt Foley added a comment - Here is a patch that provides the desired check, failing doUpgrade() if any storage directory fails. The change in FSImage is just a few lines, and easily validated by inspection. However, providing a unit test for it was very difficult. The problem is that failure must be forced within the doUpgrade() method itself, which is buried in the Namenode startup code, and quite well protected. First I tried to make the storage dir read-only, but that gets caught in recoverTransitionRead() well before invoking doUpgrade(). Second I looked at using Mockito, but it seems that in order to spy on the startup/upgrade process one would have to mock the entire stack of HDFS system objects. The invocation of NNStorage.rename() at line 367 of FSImage would be a convenient spy target, but it is static and I saw no way to get hold of it. Third, I rejected non-mock test parameters in production code. Finally I just tested it manually by temporarily hacking the code in doUpgrade() to force the error. I was able to validate my patch, and also found and fixed an NPE bug in FSEditLog.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12482900/hdfs-1955_1.patch
          against trunk revision 1136741.

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

          +1 tests included. The patch appears to include 3 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.TestDFSRollback

          +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/job/PreCommit-HDFS-Build/791//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/791//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/791//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/12482900/hdfs-1955_1.patch against trunk revision 1136741. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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.TestDFSRollback +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/job/PreCommit-HDFS-Build/791//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/791//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/791//console This message is automatically generated.
          Hide
          Matt Foley added a comment -

          While TestDFSRollback could be related to this patch, the particular error encountered doesn't make much sense. Furthermore, this unit test passes in my environment, from both the command line and eclipse.

          Resubmitting to see if it was a transient error.

          Show
          Matt Foley added a comment - While TestDFSRollback could be related to this patch, the particular error encountered doesn't make much sense. Furthermore, this unit test passes in my environment, from both the command line and eclipse. Resubmitting to see if it was a transient error.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12482966/hdfs-1955_1.patch
          against trunk revision 1136741.

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

          +1 tests included. The patch appears to include 3 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 passed core unit tests.

          +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/job/PreCommit-HDFS-Build/792//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/792//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/792//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/12482966/hdfs-1955_1.patch against trunk revision 1136741. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 passed core unit tests. +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/job/PreCommit-HDFS-Build/792//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/792//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/792//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          FSEditLog

            public synchronized void errorOccurred(StorageDirectory sd)
                throws IOException {
              if (editStreams == null) {
          

          Should this call getNumEditStreams(), or maybe even better yet isOpen() which returns false if there are no edit streams?

          FSImage

              storage.reportErrorsOnDirectories(errorSDs);
              if (!errorSDs.isEmpty()) {
          

          Would it make sense to move the call to reportErrorOnDirectories inside the if? Other callers of the method tend to not unconditionally call the method.

          This isn't strictly related to your change, but is a question/observation while tracing the code. I'm not a java threading expert, but is there a race condition here?

            private void waitForThreads(List<Thread> threads) {
              for (Thread thread : threads) {
                while (thread.isAlive()) {
                  try {
                    thread.join();
                  } catch (InterruptedException iex) {
                    LOG.error("Caught exception while waiting for thread " +
                              thread.getName() + " to finish. Retrying join");
                  }        
                }
              }
            }
          

          Can isAlive return false because the thread already terminated before waitForThreads is invoked? I ask because won't the thread be left in limbo? In which case, should the while be a do-while?

          Show
          Daryn Sharp added a comment - FSEditLog public synchronized void errorOccurred(StorageDirectory sd) throws IOException { if (editStreams == null ) { Should this call getNumEditStreams() , or maybe even better yet isOpen() which returns false if there are no edit streams? FSImage storage.reportErrorsOnDirectories(errorSDs); if (!errorSDs.isEmpty()) { Would it make sense to move the call to reportErrorOnDirectories inside the if ? Other callers of the method tend to not unconditionally call the method. This isn't strictly related to your change, but is a question/observation while tracing the code. I'm not a java threading expert, but is there a race condition here? private void waitForThreads(List< Thread > threads) { for ( Thread thread : threads) { while (thread.isAlive()) { try { thread.join(); } catch (InterruptedException iex) { LOG.error( "Caught exception while waiting for thread " + thread.getName() + " to finish. Retrying join" ); } } } } Can isAlive return false because the thread already terminated before waitForThreads is invoked? I ask because won't the thread be left in limbo? In which case, should the while be a do-while ?
          Hide
          Matt Foley added a comment -

          Should this call getNumEditStreams() [instead of "(editStreams == null)"], or maybe even better yet isOpen() which returns false if there are no edit streams?

          No, the point of this insertion is solely to prevent NPE in the rare case where (as the comment notes) an error occurs on one or more sd's before editStreams has even been initialized. The check for null is efficient and sufficient.

          Would it make sense to move the call to reportErrorOnDirectories inside the if? Other callers of the method tend to not unconditionally call the method.

          Agreed. New patch contains this change.

          is there a race condition... Can isAlive return false because the thread already terminated before waitForThreads is invoked? I ask because won't the thread be left in limbo? In which case, should the while be a do-while?

          We talked, and noted that Java thread join is not the same as pthread join. There's no race, nor other issue, because both .isAlive() and .join() can be called on an already-terminated thread without any exception being thrown. The only purpose of the loop is to deal with the possibility that an interruption may be received while this method is blocked on the join() call. It doesn't matter whether the termination condition is checked at the beginning or the end of the loop. So the existing code is acceptable.

          Show
          Matt Foley added a comment - Should this call getNumEditStreams() [instead of "(editStreams == null)"] , or maybe even better yet isOpen() which returns false if there are no edit streams? No, the point of this insertion is solely to prevent NPE in the rare case where (as the comment notes) an error occurs on one or more sd's before editStreams has even been initialized. The check for null is efficient and sufficient. Would it make sense to move the call to reportErrorOnDirectories inside the if? Other callers of the method tend to not unconditionally call the method. Agreed. New patch contains this change. is there a race condition... Can isAlive return false because the thread already terminated before waitForThreads is invoked? I ask because won't the thread be left in limbo? In which case, should the while be a do-while? We talked, and noted that Java thread join is not the same as pthread join. There's no race, nor other issue, because both .isAlive() and .join() can be called on an already-terminated thread without any exception being thrown. The only purpose of the loop is to deal with the possibility that an interruption may be received while this method is blocked on the join() call. It doesn't matter whether the termination condition is checked at the beginning or the end of the loop. So the existing code is acceptable.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12484586/hdfs-1955_2.patch
          against trunk revision 1140920.

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

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

          -1 javadoc. The javadoc tool appears to have generated 1 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 passed core unit tests.

          +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/job/PreCommit-HDFS-Build/865//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/865//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/865//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/12484586/hdfs-1955_2.patch against trunk revision 1140920. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 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 passed core unit tests. +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/job/PreCommit-HDFS-Build/865//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/865//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/865//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          +1 looks good. Thanks for the explanations!

          Show
          Daryn Sharp added a comment - +1 looks good. Thanks for the explanations!
          Hide
          Matt Foley added a comment -

          Looking at the Jenkins log, the javadoc complaint is:
          [javadoc] javadoc: warning - Error fetching URL: http://java.sun.com/javase/6/docs/api/package-list
          This is unrelated to the patch.

          Committing. Thanks, Daryn, for the review!

          Show
          Matt Foley added a comment - Looking at the Jenkins log, the javadoc complaint is: [javadoc] javadoc: warning - Error fetching URL: http://java.sun.com/javase/6/docs/api/package-list This is unrelated to the patch. Committing. Thanks, Daryn, for the review!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #769 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/769/)
          HDFS-1955. FSImage.doUpgrade() was made too fault-tolerant by HDFS-1826. Contributed by Matt Foley.

          mattf : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1141658
          Files :

          • /hadoop/common/trunk/hdfs/CHANGES.txt
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/TestDFSUpgrade.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #769 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/769/ ) HDFS-1955 . FSImage.doUpgrade() was made too fault-tolerant by HDFS-1826 . Contributed by Matt Foley. mattf : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1141658 Files : /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/TestDFSUpgrade.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #712 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/712/)
          HDFS-1955. FSImage.doUpgrade() was made too fault-tolerant by HDFS-1826. Contributed by Matt Foley.

          mattf : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1141658
          Files :

          • /hadoop/common/trunk/hdfs/CHANGES.txt
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/TestDFSUpgrade.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #712 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/712/ ) HDFS-1955 . FSImage.doUpgrade() was made too fault-tolerant by HDFS-1826 . Contributed by Matt Foley. mattf : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1141658 Files : /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/TestDFSUpgrade.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-1073-branch #9 (See https://builds.apache.org/job/Hadoop-Hdfs-1073-branch/9/)
          HDFS-2135. Fix regression of HDFS-1955 in HDFS-1073 branch. Contributed by Todd Lipcon.

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1146864
          Files :

          • /hadoop/common/branches/HDFS-1073/hdfs/CHANGES.HDFS-1073.txt
          • /hadoop/common/branches/HDFS-1073/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-1073-branch #9 (See https://builds.apache.org/job/Hadoop-Hdfs-1073-branch/9/ ) HDFS-2135 . Fix regression of HDFS-1955 in HDFS-1073 branch. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1146864 Files : /hadoop/common/branches/ HDFS-1073 /hdfs/CHANGES. HDFS-1073 .txt /hadoop/common/branches/ HDFS-1073 /hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java

            People

            • Assignee:
              Matt Foley
              Reporter:
              Matt Foley
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development