Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-2227

HDFS-2018 Part 2 : getRemoteEditLogManifest should pull it's information from FileJournalManager

    Details

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

      Description

      This is the second part of HDFS-2018. This patch moves the code that selects the available RemoteEditLogManifest out of the transactional inspector and into FileJournalManager.

      1. HDFS-2227.diff
        22 kB
        Ivan Kelly
      2. delta.txt
        23 kB
        Todd Lipcon
      3. hdfs-2227.txt
        33 kB
        Todd Lipcon
      4. hdfs-2227.txt
        37 kB
        Todd Lipcon
      5. hdfs-2227.txt
        40 kB
        Todd Lipcon
      6. hdfs-2227.txt
        34 kB
        Ivan Kelly
      7. hdfs-2227-delta.txt
        8 kB
        Ivan Kelly
      8. hdfs-2227.txt
        34 kB
        Todd Lipcon

        Issue Links

          Activity

          Ivan Kelly created issue -
          Ivan Kelly made changes -
          Field Original Value New Value
          Attachment HDFS-2227.diff [ 12489429 ]
          Ivan Kelly made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Todd Lipcon added a comment -

          Took a look through your patch and made some changes. Here's a summary of what I changed:

          • since initJournals() was now only being called from the constructor of FSEditLog, I just moved it straight in there.
          • I made getEditLogManifest synchronized, which I think was probably a bug before
          • Tightened the try/catch around fj.getRemoteEditLog since that's the only line that can actually throw - clearer to see scope of potential exceptions
          • changed log message that would result there to include the exception trace
          • added a precondition to ensure that we always find logs that start exactly at the given transaction ID. Since we currently have that as an invariant, I prefer to make it explicit everywhere rather than risk a bug where part of the system handles it and another doesn't.
          • Because of above assumption, was able to simplify the code in FileJournalManager a bit too. Since we don't use getLogFiles() anywhere else, I collapsed it with getRemoteEditLog.
          • reverted your change to make NNStorage.format(StorageDirectory) public - it's not necessary, we can use the already public format method from TestEditLog
          • changed TXNS_PER_ROLL to 10 so that the filenames are 1-10, 11-20, 21-30, etc. Just makes the test easier to look at
          • added code to dump the contents of the edits directories to the logs in this test case - made it easier to understand the tests for me.
          • some refactoring of test utils to be used from multiple places
          • added new unit test for FileJournalManager that tests the getRemoteEditLog method in isolation with a mock directory
          Show
          Todd Lipcon added a comment - Took a look through your patch and made some changes. Here's a summary of what I changed: since initJournals() was now only being called from the constructor of FSEditLog, I just moved it straight in there. I made getEditLogManifest synchronized, which I think was probably a bug before Tightened the try/catch around fj.getRemoteEditLog since that's the only line that can actually throw - clearer to see scope of potential exceptions changed log message that would result there to include the exception trace added a precondition to ensure that we always find logs that start exactly at the given transaction ID. Since we currently have that as an invariant, I prefer to make it explicit everywhere rather than risk a bug where part of the system handles it and another doesn't. Because of above assumption, was able to simplify the code in FileJournalManager a bit too. Since we don't use getLogFiles() anywhere else, I collapsed it with getRemoteEditLog. reverted your change to make NNStorage.format(StorageDirectory) public - it's not necessary, we can use the already public format method from TestEditLog changed TXNS_PER_ROLL to 10 so that the filenames are 1-10, 11-20, 21-30, etc. Just makes the test easier to look at added code to dump the contents of the edits directories to the logs in this test case - made it easier to understand the tests for me. some refactoring of test utils to be used from multiple places added new unit test for FileJournalManager that tests the getRemoteEditLog method in isolation with a mock directory
          Todd Lipcon made changes -
          Attachment delta.txt [ 12489553 ]
          Attachment hdfs-2227.txt [ 12489554 ]
          Hide
          Todd Lipcon added a comment -

          err, my patch above is broken. working on fixing it up.

          Show
          Todd Lipcon added a comment - err, my patch above is broken. working on fixing it up.
          Hide
          Todd Lipcon added a comment -

          Hey Ivan. Can you take a look at this one? It passes tests, and I think it's a little easier to understand. I changed the FileJournalManager interface to return all of the logs at once, and then do the "merging" of the different journal managers from within FSEditLog.

          I want to add a couple more strictly "unit" tests for this function before commit, but figured I'd show you the direction for now to get your thoughts.

          Show
          Todd Lipcon added a comment - Hey Ivan. Can you take a look at this one? It passes tests, and I think it's a little easier to understand. I changed the FileJournalManager interface to return all of the logs at once, and then do the "merging" of the different journal managers from within FSEditLog. I want to add a couple more strictly "unit" tests for this function before commit, but figured I'd show you the direction for now to get your thoughts.
          Todd Lipcon made changes -
          Attachment hdfs-2227.txt [ 12489722 ]
          Hide
          Todd Lipcon added a comment -

          Here's the addition of unit tests.

          Since these true unit tests cover the same thing as the more complicated "AbortSpec" based test, maybe we can convert that one over? At least I find these new ones easier to read.

          Show
          Todd Lipcon added a comment - Here's the addition of unit tests. Since these true unit tests cover the same thing as the more complicated "AbortSpec" based test, maybe we can convert that one over? At least I find these new ones easier to read.
          Todd Lipcon made changes -
          Attachment hdfs-2227.txt [ 12489748 ]
          Hide
          Ivan Kelly added a comment -

          This approach is fine with me, as long as it doesn't modify the general FileJournalManager interface. I've converted the second AbortSpec test to the mock format. The first one was already covered in the mock test.

          Show
          Ivan Kelly added a comment - This approach is fine with me, as long as it doesn't modify the general FileJournalManager interface. I've converted the second AbortSpec test to the mock format. The first one was already covered in the mock test.
          Ivan Kelly made changes -
          Attachment hdfs-2227.txt [ 12489843 ]
          Attachment hdfs-2227-delta.txt [ 12489844 ]
          Hide
          Ivan Kelly added a comment -

          s/FileJournalManager/JournalManager/

          Show
          Ivan Kelly added a comment - s/FileJournalManager/JournalManager/
          Ivan Kelly made changes -
          Link This issue is required by HDFS-2225 [ HDFS-2225 ]
          Ivan Kelly made changes -
          Link This issue is required by HDFS-2225 [ HDFS-2225 ]
          Ivan Kelly made changes -
          Link This issue is required by HDFS-2018 [ HDFS-2018 ]
          Hide
          Jitendra Nath Pandey added a comment -

          The latest hdfs-2227.txt patch looks good to me. +1

          Show
          Jitendra Nath Pandey added a comment - The latest hdfs-2227.txt patch looks good to me. +1
          Hide
          Todd Lipcon added a comment -

          Cool, I will run test-patch and some related unit tests, and commit if it passes

          Show
          Todd Lipcon added a comment - Cool, I will run test-patch and some related unit tests, and commit if it passes
          Hide
          Todd Lipcon added a comment -

          The patch had one javadoc warning (use of "@see" instead of "@link" in RemoteEditLog) and one Findbugs warning (missing hashCode implementation in RemoteEditLog).

          The patch attached fixes those and now comes back clean:

          [exec]
          [exec] +1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 20 new or modified tests.
          [exec]
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec]
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.8) warnings.
          [exec]
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
          [exec]
          [exec] +1 system test framework. The patch passed system test framework compile.

          Running some unit tests now.

          Show
          Todd Lipcon added a comment - The patch had one javadoc warning (use of "@see" instead of "@link" in RemoteEditLog) and one Findbugs warning (missing hashCode implementation in RemoteEditLog). The patch attached fixes those and now comes back clean: [exec] [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 20 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.8) warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] +1 system test framework. The patch passed system test framework compile. Running some unit tests now.
          Todd Lipcon made changes -
          Attachment hdfs-2227.txt [ 12489908 ]
          Hide
          Todd Lipcon added a comment -

          Committed to trunk. Thanks for reviewing, Jitendra, and for the contribution, Ivan.

          Show
          Todd Lipcon added a comment - Committed to trunk. Thanks for reviewing, Jitendra, and for the contribution, Ivan.
          Todd Lipcon made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Fix Version/s 0.23.0 [ 12315571 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #717 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/717/)
          HDFS-2227. getRemoteEditLogManifest should pull its information from FileJournalManager during checkpoint process. Contributed by Ivan Kelly and Todd Lipcon.

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

          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/FSImageTestUtil.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImageTransactionalStorageInspector.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/Checkpointer.java
          • /hadoop/common/trunk/hdfs/CHANGES.txt
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestCheckpoint.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FileJournalManager.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/RemoteEditLog.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStorageRestore.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestNNStorageRetentionManager.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFileJournalManager.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFSImageStorageInspector.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/BackupImage.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #717 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/717/ ) HDFS-2227 . getRemoteEditLogManifest should pull its information from FileJournalManager during checkpoint process. Contributed by Ivan Kelly and Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1155977 Files : /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/FSImageTestUtil.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImageTransactionalStorageInspector.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/Checkpointer.java /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestCheckpoint.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FileJournalManager.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/RemoteEditLog.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStorageRestore.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestNNStorageRetentionManager.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFileJournalManager.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFSImageStorageInspector.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/BackupImage.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #820 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/820/)
          HDFS-2227. getRemoteEditLogManifest should pull its information from FileJournalManager during checkpoint process. Contributed by Ivan Kelly and Todd Lipcon.

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

          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/FSImageTestUtil.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImageTransactionalStorageInspector.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/Checkpointer.java
          • /hadoop/common/trunk/hdfs/CHANGES.txt
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestCheckpoint.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FileJournalManager.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/RemoteEditLog.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStorageRestore.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestNNStorageRetentionManager.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFileJournalManager.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFSImageStorageInspector.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/BackupImage.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #820 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/820/ ) HDFS-2227 . getRemoteEditLogManifest should pull its information from FileJournalManager during checkpoint process. Contributed by Ivan Kelly and Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1155977 Files : /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/FSImageTestUtil.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImageTransactionalStorageInspector.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/Checkpointer.java /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestCheckpoint.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FileJournalManager.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/RemoteEditLog.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStorageRestore.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestNNStorageRetentionManager.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFileJournalManager.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFSImageStorageInspector.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/BackupImage.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #745 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/745/)
          HDFS-2227. getRemoteEditLogManifest should pull its information from FileJournalManager during checkpoint process. Contributed by Ivan Kelly and Todd Lipcon.

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

          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/FSImageTestUtil.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImageTransactionalStorageInspector.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/Checkpointer.java
          • /hadoop/common/trunk/hdfs/CHANGES.txt
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestCheckpoint.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FileJournalManager.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/RemoteEditLog.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStorageRestore.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestNNStorageRetentionManager.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFileJournalManager.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFSImageStorageInspector.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/BackupImage.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #745 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/745/ ) HDFS-2227 . getRemoteEditLogManifest should pull its information from FileJournalManager during checkpoint process. Contributed by Ivan Kelly and Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1155977 Files : /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/FSImageTestUtil.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImageTransactionalStorageInspector.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/Checkpointer.java /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestCheckpoint.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FileJournalManager.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/RemoteEditLog.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStorageRestore.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestNNStorageRetentionManager.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFileJournalManager.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFSImageStorageInspector.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/BackupImage.java
          Arun C Murthy made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Ivan Kelly
              Reporter:
              Ivan Kelly
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development