Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-1505

saveNamespace appears to succeed even if all directories fail to save

    Details

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

      Description

      After HDFS-1071, saveNamespace now appears to "succeed" even if all of the individual directories failed to save.

      1. hdfs-1505-22.2.patch
        4 kB
        Aaron T. Myers
      2. hdfs-1505-trunk.3.patch
        4 kB
        Aaron T. Myers
      3. hdfs-1505-trunk.2.patch
        4 kB
        Aaron T. Myers
      4. hdfs-1505-1-test.txt
        3 kB
        Matt Foley
      5. hdfs-1505-trunk.1.patch
        3 kB
        Aaron T. Myers
      6. hdfs-1505-22.1.patch
        3 kB
        Aaron T. Myers
      7. hdfs-1505-trunk.0.patch
        3 kB
        Aaron T. Myers
      8. hdfs-1505-22.0.patch
        3 kB
        Aaron T. Myers
      9. hdfs-1505-test.txt
        3 kB
        Todd Lipcon

        Issue Links

          Activity

          Konstantin Shvachko made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Matt Foley made changes -
          Fix Version/s 0.23.0 [ 12315571 ]
          Hide
          Hudson added a comment -

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

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

          Hey Matt. You're pretty close.

          And you're referring to the resulting fact that one could take an image ending with txn 100, jump into the middle of a log file that went from txn 50 to 170

          In theory, yes. In the current implementation, images are only saved at boundaries of edit log segments. So if you have an image with txn 100, then you'll have some edit log file which starts at 101, so the "jump into the middle" part isn't necessary.

          Although it doesn't hurt to also clear the edits logs, once you have multiple copies of the fsimage. Does your log-rolling logic automatically delete log chunk files older than available fsimage files?

          It's not implemented yet, but the idea is that a separate background thread would be responsible for handling management of old files based on various policies (eg remove old ones, or perhaps archive to some other location)

          So, sounds like we're in agreement. Thanks.

          Show
          Todd Lipcon added a comment - Hey Matt. You're pretty close. And you're referring to the resulting fact that one could take an image ending with txn 100, jump into the middle of a log file that went from txn 50 to 170 In theory, yes. In the current implementation, images are only saved at boundaries of edit log segments. So if you have an image with txn 100, then you'll have some edit log file which starts at 101, so the "jump into the middle" part isn't necessary. Although it doesn't hurt to also clear the edits logs, once you have multiple copies of the fsimage. Does your log-rolling logic automatically delete log chunk files older than available fsimage files? It's not implemented yet, but the idea is that a separate background thread would be responsible for handling management of old files based on various policies (eg remove old ones, or perhaps archive to some other location) So, sounds like we're in agreement. Thanks.
          Hide
          Matt Foley added a comment -

          Reading the HDFS-1073 spec, I infer that fsimage files will have a tag identifying the last txn included in the image, and edits logs will have tags for the first and last txn included in them. And you're referring to the resulting fact that one could take an image ending with txn 100, jump into the middle of a log file that went from txn 50 to 170, and successfully generate the in-memory structures current as of txn 170. Is that right?

          If the above understanding is correct, then I agree it seems that saveNamespace() should just save the fsimage file. Although it doesn't hurt to also clear the edits logs, once you have multiple copies of the fsimage. Does your log-rolling logic automatically delete log chunk files older than available fsimage files? That would be sufficient edits file management.

          Show
          Matt Foley added a comment - Reading the HDFS-1073 spec, I infer that fsimage files will have a tag identifying the last txn included in the image, and edits logs will have tags for the first and last txn included in them. And you're referring to the resulting fact that one could take an image ending with txn 100, jump into the middle of a log file that went from txn 50 to 170, and successfully generate the in-memory structures current as of txn 170. Is that right? If the above understanding is correct, then I agree it seems that saveNamespace() should just save the fsimage file. Although it doesn't hurt to also clear the edits logs, once you have multiple copies of the fsimage. Does your log-rolling logic automatically delete log chunk files older than available fsimage files? That would be sufficient edits file management.
          Hide
          Todd Lipcon added a comment -

          I'm working on porting this over to the branch for HDFS-1073... quick question for you guys:
          In the HDFS-1073 design, the edits and the images are no longer coupled – ie there's no coordination between the state of the edits files and of the images. Given this, I think the correct check will be to see if there are any IMAGE directories at the end of saveNamespace, and not care about EDITS (we're also decoupling failure state of the edit log from failure state of the image saving). Does that seem to make sense to you? The tests pass for me on that branch even with that change.

          Show
          Todd Lipcon added a comment - I'm working on porting this over to the branch for HDFS-1073 ... quick question for you guys: In the HDFS-1073 design, the edits and the images are no longer coupled – ie there's no coordination between the state of the edits files and of the images. Given this, I think the correct check will be to see if there are any IMAGE directories at the end of saveNamespace, and not care about EDITS (we're also decoupling failure state of the edit log from failure state of the image saving). Does that seem to make sense to you? The tests pass for me on that branch even with that change.
          Hide
          Matt Foley added a comment -

          The same issue applies to doUpgrade(), as mentioned above.
          Opened new Jira HDFS-1955. (The unit test made it too complex to include in HDFS-1921.)

          Show
          Matt Foley added a comment - The same issue applies to doUpgrade(), as mentioned above. Opened new Jira HDFS-1955 . (The unit test made it too complex to include in HDFS-1921 .)
          Matt Foley made changes -
          Link This issue is cloned as HDFS-1955 [ HDFS-1955 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-22-branch #49 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-22-branch/49/)
          HDFS-1505. saveNamespace appears to succeed even if all directories fail to save. Contributed by Aaron T. Myers.

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

          • /hadoop/hdfs/branches/branch-0.22/src/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java
          • /hadoop/hdfs/branches/branch-0.22/CHANGES.txt
          • /hadoop/hdfs/branches/branch-0.22/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestSaveNamespace.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-22-branch #49 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-22-branch/49/ ) HDFS-1505 . saveNamespace appears to succeed even if all directories fail to save. Contributed by Aaron T. Myers. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1104685 Files : /hadoop/hdfs/branches/branch-0.22/src/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java /hadoop/hdfs/branches/branch-0.22/CHANGES.txt /hadoop/hdfs/branches/branch-0.22/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestSaveNamespace.java
          Todd Lipcon made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Resolution Fixed [ 1 ]
          Hide
          Todd Lipcon added a comment -

          Committed the 0.22 patch as well. Thanks Aaron and Matt!

          Show
          Todd Lipcon added a comment - Committed the 0.22 patch as well. Thanks Aaron and Matt!
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12479538/hdfs-1505-22.2.patch
          against trunk revision 1104649.

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

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

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

          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/552//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/12479538/hdfs-1505-22.2.patch against trunk revision 1104649. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/552//console This message is automatically generated.
          Hide
          Hudson added a comment -

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

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #665 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/665/ )
          Aaron T. Myers made changes -
          Attachment hdfs-1505-22.2.patch [ 12479538 ]
          Hide
          Aaron T. Myers added a comment -

          Aaron: is the 0.22 patch up to date with the latest feedback? Which one should I commit?

          It was not. You should commit the one I'm attaching right now.

          Show
          Aaron T. Myers added a comment - Aaron: is the 0.22 patch up to date with the latest feedback? Which one should I commit? It was not. You should commit the one I'm attaching right now.
          Hide
          Todd Lipcon added a comment -

          Committed the trunk patch. Aaron: is the 0.22 patch up to date with the latest feedback? Which one should I commit?

          Show
          Todd Lipcon added a comment - Committed the trunk patch. Aaron: is the 0.22 patch up to date with the latest feedback? Which one should I commit?
          Matt Foley made changes -
          Link This issue is cloned as HDFS-1952 [ HDFS-1952 ]
          Hide
          Matt Foley added a comment -

          +1 Looks good to me!

          Show
          Matt Foley added a comment - +1 Looks good to me!
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12479421/hdfs-1505-trunk.3.patch
          against trunk revision 1103987.

          +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.TestDFSStorageStateRecovery
          org.apache.hadoop.hdfs.TestFileConcurrentReader
          org.apache.hadoop.hdfs.TestHDFSTrash
          org.apache.hadoop.tools.TestJMXGet

          +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/539//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/539//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/539//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/12479421/hdfs-1505-trunk.3.patch against trunk revision 1103987. +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.TestDFSStorageStateRecovery org.apache.hadoop.hdfs.TestFileConcurrentReader org.apache.hadoop.hdfs.TestHDFSTrash org.apache.hadoop.tools.TestJMXGet +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/539//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/539//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/539//console This message is automatically generated.
          Aaron T. Myers made changes -
          Attachment hdfs-1505-trunk.3.patch [ 12479421 ]
          Hide
          Aaron T. Myers added a comment -

          Updated patch addressing Matt's comments AND fixing up the test.

          Show
          Aaron T. Myers added a comment - Updated patch addressing Matt's comments AND fixing up the test.
          Hide
          Aaron T. Myers added a comment -

          Hi Aaron, sorry to keep harping on this (and this was likely a typo), but the test needs to be

          This was my goof. I made the test changes on the trunk branch but got mixed up and made the changes to address your comments on my 0.22 branch. I'll attach a patch shortly with all the changes.

          The TestSaveNamespace mod looks good.

          Thanks.

          Are you still going to address this one?

          I would suggest fixing the lack of notification in FSEditLog.open(), but also in your patch to saveNamespace() the check for empty IMAGE and EDITS lists should precede the call to editLog.open().

          Yep - same deal as the above.

          BTW, after thinking about your comment, I think I will change doUpgrade() to fail on any bad storage dir, the way it used to before HDFS-1826. I'll do that under HDFS-1921 since I'm in FSImage anyway. Sound okay?

          That sounds fine to me.

          Show
          Aaron T. Myers added a comment - Hi Aaron, sorry to keep harping on this (and this was likely a typo), but the test needs to be This was my goof. I made the test changes on the trunk branch but got mixed up and made the changes to address your comments on my 0.22 branch. I'll attach a patch shortly with all the changes. The TestSaveNamespace mod looks good. Thanks. Are you still going to address this one? I would suggest fixing the lack of notification in FSEditLog.open(), but also in your patch to saveNamespace() the check for empty IMAGE and EDITS lists should precede the call to editLog.open(). Yep - same deal as the above. BTW, after thinking about your comment, I think I will change doUpgrade() to fail on any bad storage dir, the way it used to before HDFS-1826 . I'll do that under HDFS-1921 since I'm in FSImage anyway. Sound okay? That sounds fine to me.
          Hide
          Matt Foley added a comment -

          Hi Aaron, sorry to keep harping on this (and this was likely a typo), but the test needs to be

          +    if (storage.getNumStorageDirs(NameNodeDirType.IMAGE) == 0 ||
          +        storage.getNumStorageDirs(NameNodeDirType.EDITS) == 0) {
          +      throw new IOException("Failed to save at least one storage directory for each of IMAGE and EDITS while saving namespace");
          

          The current patch's test, (num(IMAGE) == 0 || num(IMAGE_AND_EDITS) == 0) could fail false-positive, by not detecting a valid EDITS-only directory.

          The TestSaveNamespace mod looks good.

          Are you still going to address this one?

          I would suggest fixing the lack of notification in FSEditLog.open(), but also in your patch to saveNamespace() the check for empty IMAGE and EDITS lists should precede the call to editLog.open().

          Thanks.

          BTW, after thinking about your comment, I think I will change doUpgrade() to fail on any bad storage dir, the way it used to before HDFS-1826. I'll do that under HDFS-1921 since I'm in FSImage anyway. Sound okay?

          Show
          Matt Foley added a comment - Hi Aaron, sorry to keep harping on this (and this was likely a typo), but the test needs to be + if (storage.getNumStorageDirs(NameNodeDirType.IMAGE) == 0 || + storage.getNumStorageDirs(NameNodeDirType.EDITS) == 0) { + throw new IOException( "Failed to save at least one storage directory for each of IMAGE and EDITS while saving namespace" ); The current patch's test, (num(IMAGE) == 0 || num(IMAGE_AND_EDITS) == 0) could fail false-positive, by not detecting a valid EDITS-only directory. The TestSaveNamespace mod looks good. Are you still going to address this one? I would suggest fixing the lack of notification in FSEditLog.open(), but also in your patch to saveNamespace() the check for empty IMAGE and EDITS lists should precede the call to editLog.open(). Thanks. BTW, after thinking about your comment, I think I will change doUpgrade() to fail on any bad storage dir, the way it used to before HDFS-1826 . I'll do that under HDFS-1921 since I'm in FSImage anyway. Sound okay?
          Aaron T. Myers made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Aaron T. Myers added a comment -

          The inclination of our team is leave the behavior unchanged here, and open another Jira for that discussion.

          The inclination of my ops team is similar. Will punt on this for the purpose of this JIRA.

          Show
          Aaron T. Myers added a comment - The inclination of our team is leave the behavior unchanged here, and open another Jira for that discussion. The inclination of my ops team is similar. Will punt on this for the purpose of this JIRA.
          Aaron T. Myers made changes -
          Attachment hdfs-1505-trunk.2.patch [ 12479409 ]
          Hide
          Aaron T. Myers added a comment -

          Updated patch which addresses Matt's review comments and fixes up the test.

          Show
          Aaron T. Myers added a comment - Updated patch which addresses Matt's review comments and fixes up the test.
          Hide
          Matt Foley added a comment -

          Correction: HADOOP-4885, not HDFS.

          Show
          Matt Foley added a comment - Correction: HADOOP-4885 , not HDFS.
          Hide
          Matt Foley added a comment -

          ...failure handling should perhaps be different between these two cases [saveNamespace and doUpgrade]

          The inclination of our team is leave the behavior unchanged here, and open another Jira for that discussion.

          Historical info:

          • A quick review of the patches for HDFS-1071 and HDFS-1826 indicates that prior to making FSImage write concurrent, saveNamespace logged storage directory failures and continued, but doUpgrade killed the Namenode on any failure.
          • With the concurrent write code, both now log and continue. This may be a deficiency in my HDFS-1826 patch.
          • HDFS-4885 introduced the ability to recover from transient storage dir failures.
          Show
          Matt Foley added a comment - ...failure handling should perhaps be different between these two cases [saveNamespace and doUpgrade] The inclination of our team is leave the behavior unchanged here, and open another Jira for that discussion. Historical info: A quick review of the patches for HDFS-1071 and HDFS-1826 indicates that prior to making FSImage write concurrent, saveNamespace logged storage directory failures and continued, but doUpgrade killed the Namenode on any failure. With the concurrent write code, both now log and continue. This may be a deficiency in my HDFS-1826 patch. HDFS-4885 introduced the ability to recover from transient storage dir failures.
          Matt Foley made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Matt Foley made changes -
          Attachment hdfs-1505-1-test.txt [ 12479028 ]
          Hide
          Matt Foley added a comment -

          Here's a modified form of the additional test case, that resolves a failure to unlock the storage dirs upon fsn.close().

          Canceling patch to avoid spurious Hudson run.

          Show
          Matt Foley added a comment - Here's a modified form of the additional test case, that resolves a failure to unlock the storage dirs upon fsn.close(). Canceling patch to avoid spurious Hudson run.
          Hide
          Matt Foley added a comment -

          Good question. I don't know. Let's both ask our ops teams.

          Show
          Matt Foley added a comment - Good question. I don't know. Let's both ask our ops teams.
          Hide
          Aaron T. Myers added a comment -

          That is, isOfType() is permissive rather than exclusive.

          You are quite correct. My mistake. The original logic you posted for the check seems to be correct.

          Also, since HDFS-1826 copied the concurrent saveNamespace() logic into FSImage.doUpgrade(), would you please add the same code fragment to the end of doUpgrade(), and a corresponding corruption unit test case to TestDFSUpgrade? Thanks.

          It occurs to me now that the failure handling should perhaps be different between these two cases. i.e. it is acceptable to tolerate some number of storage directory failures during save namespace, but we should perhaps throw an error in the event any storage directories fail during upgrade. Thoughts?

          Show
          Aaron T. Myers added a comment - That is, isOfType() is permissive rather than exclusive. You are quite correct. My mistake. The original logic you posted for the check seems to be correct. Also, since HDFS-1826 copied the concurrent saveNamespace() logic into FSImage.doUpgrade(), would you please add the same code fragment to the end of doUpgrade(), and a corresponding corruption unit test case to TestDFSUpgrade? Thanks. It occurs to me now that the failure handling should perhaps be different between these two cases. i.e. it is acceptable to tolerate some number of storage directory failures during save namespace, but we should perhaps throw an error in the event any storage directories fail during upgrade. Thoughts?
          Hide
          Matt Foley added a comment -

          Hi Aaron, agree with you that storage directories of type IMAGE_AND_EDITS are a distinct NameNodeDirType. However, my understanding of NNStorage.getNumStorageDirs(NameNodeDirType), and NameNodeDirType.isOfType() is that membership queries (iterators or counts) about storage dirs of type EDITS return answers relating to all storage dirs of type EDITS || IMAGE_AND_EDITS, while queries about storage dirs of type IMAGE return answers relating to all storage dirs of type IMAGE || IMAGE_AND_EDITS. That is, isOfType() is permissive rather than exclusive.

          I could be wrong of course as it's possible I didn't correctly follow overloaded implementations. Please let me know if so. Thanks.

          Show
          Matt Foley added a comment - Hi Aaron, agree with you that storage directories of type IMAGE_AND_EDITS are a distinct NameNodeDirType. However, my understanding of NNStorage.getNumStorageDirs(NameNodeDirType), and NameNodeDirType.isOfType() is that membership queries (iterators or counts) about storage dirs of type EDITS return answers relating to all storage dirs of type EDITS || IMAGE_AND_EDITS, while queries about storage dirs of type IMAGE return answers relating to all storage dirs of type IMAGE || IMAGE_AND_EDITS. That is, isOfType() is permissive rather than exclusive. I could be wrong of course as it's possible I didn't correctly follow overloaded implementations. Please let me know if so. Thanks.
          Aaron T. Myers made changes -
          Link This issue relates to HDFS-1896 [ HDFS-1896 ]
          Hide
          Aaron T. Myers added a comment -

          Also, per talking to Todd, I believe this work will be obsoleted for 0.23 by the work in HDFS-1073. As such, I'm only going to prepare a patch for 0.22.

          Show
          Aaron T. Myers added a comment - Also, per talking to Todd, I believe this work will be obsoleted for 0.23 by the work in HDFS-1073 . As such, I'm only going to prepare a patch for 0.22.
          Hide
          Aaron T. Myers added a comment -

          Thanks a lot for the review/comments, Matt.

          Upon further reflection, I think the desired check should actually be:

              if ((storage.getNumStorageDirs(NameNodeDirType.IMAGE) == 0 ||
                  storage.getNumStorageDirs(NameNodeDirType.EDITS) == 0) &&
                  storage.getNumStorageDirs(NameNodeDirType.IMAGE_AND_EDITS) == 0) {
                throw new IOException("Failed to save any storage directories while saving namespace");
          

          What do you think? Note that IMAGE_AND_EDITS is a distinct type of storage directory, which contains both fsimage and edits files. Apologies if you already knew that.

          Also, since HDFS-1826 copied the concurrent saveNamespace() logic into FSImage.doUpgrade(), would you please add the same code fragment to the end of doUpgrade(), and a corresponding corruption unit test case to TestDFSUpgrade? Thanks.

          Will do.

          I would suggest fixing the lack of notification in FSEditLog.open(), but also in your patch to saveNamespace() the check for empty IMAGE and EDITS lists should precede the call to editLog.open().

          Agreed. Will do.

          Show
          Aaron T. Myers added a comment - Thanks a lot for the review/comments, Matt. Upon further reflection, I think the desired check should actually be: if ((storage.getNumStorageDirs(NameNodeDirType.IMAGE) == 0 || storage.getNumStorageDirs(NameNodeDirType.EDITS) == 0) && storage.getNumStorageDirs(NameNodeDirType.IMAGE_AND_EDITS) == 0) { throw new IOException( "Failed to save any storage directories while saving namespace" ); What do you think? Note that IMAGE_AND_EDITS is a distinct type of storage directory, which contains both fsimage and edits files. Apologies if you already knew that. Also, since HDFS-1826 copied the concurrent saveNamespace() logic into FSImage.doUpgrade(), would you please add the same code fragment to the end of doUpgrade(), and a corresponding corruption unit test case to TestDFSUpgrade? Thanks. Will do. I would suggest fixing the lack of notification in FSEditLog.open(), but also in your patch to saveNamespace() the check for empty IMAGE and EDITS lists should precede the call to editLog.open(). Agreed. Will do.
          Hide
          Matt Foley added a comment -

          One more related issue: at the end of saveNamespace() it calls "editLog.open()", which is implemented by FSEditLog.open(). This routine has the same problem: if the list of EditLogOutputStream is empty, it appears to succeed, but should throw an exception.

          I would suggest fixing the lack of notification in FSEditLog.open(), but also in your patch to saveNamespace() the check for empty IMAGE and EDITS lists should precede the call to editLog.open().

          Show
          Matt Foley added a comment - One more related issue: at the end of saveNamespace() it calls "editLog.open()", which is implemented by FSEditLog.open(). This routine has the same problem: if the list of EditLogOutputStream is empty, it appears to succeed, but should throw an exception. I would suggest fixing the lack of notification in FSEditLog.open(), but also in your patch to saveNamespace() the check for empty IMAGE and EDITS lists should precede the call to editLog.open().
          Hide
          Matt Foley added a comment -

          Regarding the check for

          +    if (storage.getNumStorageDirs(NameNodeDirType.IMAGE) == 0 &&
          +        storage.getNumStorageDirs(NameNodeDirType.IMAGE_AND_EDITS) == 0) {
          +      throw new IOException("Failed to save any storage directories while saving namespace");
          

          Isn't the desired check actually

          +    if (storage.getNumStorageDirs(NameNodeDirType.IMAGE) == 0 ||
          +        storage.getNumStorageDirs(NameNodeDirType.EDITS) == 0) {
          +      throw new IOException("Failed to save at least one storage directory for both IMAGE and EDITS while saving namespace");
          

          Also, since HDFS-1826 copied the concurrent saveNamespace() logic into FSImage.doUpgrade(), would you please add the same code fragment to the end of doUpgrade(), and a corresponding corruption unit test case to TestDFSUpgrade? Thanks.

          Show
          Matt Foley added a comment - Regarding the check for + if (storage.getNumStorageDirs(NameNodeDirType.IMAGE) == 0 && + storage.getNumStorageDirs(NameNodeDirType.IMAGE_AND_EDITS) == 0) { + throw new IOException( "Failed to save any storage directories while saving namespace" ); Isn't the desired check actually + if (storage.getNumStorageDirs(NameNodeDirType.IMAGE) == 0 || + storage.getNumStorageDirs(NameNodeDirType.EDITS) == 0) { + throw new IOException( "Failed to save at least one storage directory for both IMAGE and EDITS while saving namespace" ); Also, since HDFS-1826 copied the concurrent saveNamespace() logic into FSImage.doUpgrade(), would you please add the same code fragment to the end of doUpgrade(), and a corresponding corruption unit test case to TestDFSUpgrade? Thanks.
          Hide
          Aaron T. Myers added a comment -

          I believe the test failures are unrelated. All of these are presently failing on trunk.

          Show
          Aaron T. Myers added a comment - I believe the test failures are unrelated. All of these are presently failing on trunk.
          Hide
          Hadoop QA added a comment -

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

          +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.cli.TestHDFSCLI
          org.apache.hadoop.hdfs.TestDFSShell
          org.apache.hadoop.hdfs.TestDFSStorageStateRecovery
          org.apache.hadoop.hdfs.TestFileConcurrentReader
          org.apache.hadoop.hdfs.TestHDFSTrash
          org.apache.hadoop.tools.TestJMXGet

          +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/495//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/495//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/495//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/12478913/hdfs-1505-trunk.1.patch against trunk revision 1102153. +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.cli.TestHDFSCLI org.apache.hadoop.hdfs.TestDFSShell org.apache.hadoop.hdfs.TestDFSStorageStateRecovery org.apache.hadoop.hdfs.TestFileConcurrentReader org.apache.hadoop.hdfs.TestHDFSTrash org.apache.hadoop.tools.TestJMXGet +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/495//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/495//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/495//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/12478913/hdfs-1505-trunk.1.patch
          against trunk revision 1102153.

          +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.cli.TestHDFSCLI
          org.apache.hadoop.hdfs.TestDFSShell
          org.apache.hadoop.hdfs.TestDFSStorageStateRecovery
          org.apache.hadoop.hdfs.TestFileConcurrentReader
          org.apache.hadoop.tools.TestJMXGet

          +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/492//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/492//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/492//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/12478913/hdfs-1505-trunk.1.patch against trunk revision 1102153. +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.cli.TestHDFSCLI org.apache.hadoop.hdfs.TestDFSShell org.apache.hadoop.hdfs.TestDFSStorageStateRecovery org.apache.hadoop.hdfs.TestFileConcurrentReader org.apache.hadoop.tools.TestJMXGet +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/492//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/492//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/492//console This message is automatically generated.
          Todd Lipcon made changes -
          Assignee Jakob Homan [ jghoman ] Aaron T. Myers [ atm ]
          Todd Lipcon made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Aaron T. Myers made changes -
          Attachment hdfs-1505-22.1.patch [ 12478912 ]
          Attachment hdfs-1505-trunk.1.patch [ 12478913 ]
          Hide
          Aaron T. Myers added a comment -

          Thanks a lot for the review, Todd. I've attached updated patches incorporating your comments.

          Show
          Aaron T. Myers added a comment - Thanks a lot for the review, Todd. I've attached updated patches incorporating your comments.
          Hide
          Todd Lipcon added a comment -

          + if (storage.getNumStorageDirs(NameNodeDirType.IMAGE) == 0 &&
          + storage.getNumStorageDirs(NameNodeDirType.EDITS) == 0 &&
          + storage.getNumStorageDirs(NameNodeDirType.IMAGE_AND_EDITS) == 0) {
          + throw new IOException("Failed to save any storage directories while saving namespace");

          I think the check for .EDITS is incorrect – if we lose all the IMAGE and IMAGE_AND_EDITS dirs, but still have EDITS dirs, we're still screwed.

          Show
          Todd Lipcon added a comment - + if (storage.getNumStorageDirs(NameNodeDirType.IMAGE) == 0 && + storage.getNumStorageDirs(NameNodeDirType.EDITS) == 0 && + storage.getNumStorageDirs(NameNodeDirType.IMAGE_AND_EDITS) == 0) { + throw new IOException("Failed to save any storage directories while saving namespace"); I think the check for .EDITS is incorrect – if we lose all the IMAGE and IMAGE_AND_EDITS dirs, but still have EDITS dirs, we're still screwed.
          Hide
          Aaron T. Myers added a comment -

          Jakob, do you mind if I reassign this JIRA to myself?

          Show
          Aaron T. Myers added a comment - Jakob, do you mind if I reassign this JIRA to myself?
          Aaron T. Myers made changes -
          Attachment hdfs-1505-trunk.0.patch [ 12478911 ]
          Hide
          Aaron T. Myers added a comment -

          Patch for trunk.

          Show
          Aaron T. Myers added a comment - Patch for trunk.
          Aaron T. Myers made changes -
          Attachment hdfs-1505-22.0.patch [ 12478906 ]
          Hide
          Aaron T. Myers added a comment -

          Patch for 0.22. Note that I commented-out a section of Todd's original test since it won't pass until HDFS-1921 is fixed.

          Show
          Aaron T. Myers added a comment - Patch for 0.22. Note that I commented-out a section of Todd's original test since it won't pass until HDFS-1921 is fixed.
          Hide
          Jakob Homan added a comment -

          Fell off my radar. Pinning it back up.

          Show
          Jakob Homan added a comment - Fell off my radar. Pinning it back up.
          Hide
          Nigel Daley added a comment -

          Jakob, any update on this for 0.22?

          Show
          Nigel Daley added a comment - Jakob, any update on this for 0.22?
          Hide
          Allen Wittenauer added a comment -

          > I agree saveNamespace should throw an exception if it fails all to save to any image directory.

          If that kills the namenode, then no, definitely not. I'd much rather have the discretion to schedule the NN shutdown rather than have it just die. Yes, there is a risk with only one image being active. But that should be an ops call, not a dev one.

          Show
          Allen Wittenauer added a comment - > I agree saveNamespace should throw an exception if it fails all to save to any image directory. If that kills the namenode, then no, definitely not. I'd much rather have the discretion to schedule the NN shutdown rather than have it just die. Yes, there is a risk with only one image being active. But that should be an ops call, not a dev one.
          Hide
          Jakob Homan added a comment -

          Yes, I'm hoping to have a patch for this this week.

          Show
          Jakob Homan added a comment - Yes, I'm hoping to have a patch for this this week.
          Nigel Daley made changes -
          Fix Version/s 0.22.0 [ 12314241 ]
          Hide
          Nigel Daley added a comment -

          Hi Jakob, are you working on a patch for this for 0.22? If so, many thanks! I'm going to mark this for 0.22.

          Show
          Nigel Daley added a comment - Hi Jakob, are you working on a patch for this for 0.22? If so, many thanks! I'm going to mark this for 0.22.
          Jakob Homan made changes -
          Assignee Jakob Homan [ jghoman ]
          Hide
          Todd Lipcon added a comment -

          Just commented out the other tests to show which one is actually the failure I'm talking about – this patch doesn't fix anything, just shows the problem.

          Show
          Todd Lipcon added a comment - Just commented out the other tests to show which one is actually the failure I'm talking about – this patch doesn't fix anything, just shows the problem.
          Hide
          Eli Collins added a comment -

          I agree saveNamespace should throw an exception if it fails all to save to any image directory.

          Show
          Eli Collins added a comment - I agree saveNamespace should throw an exception if it fails all to save to any image directory.
          Hide
          Eli Collins added a comment -

          Patch looks good. Why comment out the three tests that use saveNamespaceWithInjectedFault?

          Show
          Eli Collins added a comment - Patch looks good. Why comment out the three tests that use saveNamespaceWithInjectedFault?
          Todd Lipcon made changes -
          Field Original Value New Value
          Attachment hdfs-1505-test.txt [ 12459829 ]
          Hide
          Todd Lipcon added a comment -

          here's a test that shows the problem.

          If all of the image directories fail to saveNamespace, saveNamespace itself should probably throw an exception, no?

          Show
          Todd Lipcon added a comment - here's a test that shows the problem. If all of the image directories fail to saveNamespace, saveNamespace itself should probably throw an exception, no?
          Hide
          Todd Lipcon added a comment -

          Actually looks like this isn't new in HDFS-1071 - perhaps new in HDFS-955 or perhaps been here forever.

          Show
          Todd Lipcon added a comment - Actually looks like this isn't new in HDFS-1071 - perhaps new in HDFS-955 or perhaps been here forever.
          Todd Lipcon created issue -

            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