Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-2187

HDFS-1580: Make EditLogInputStream act like an iterator over FSEditLogOps

    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 JIRA is for the input side changes moved out of HDFS-2149. EditLogInputStream has been changed to no longer be an InputStream implementation, but to return a stream of FSEditLogOp objects using readOp(). The upshot is that all that can ever be read from an EditLogInputStream is a op. No random hackery can be used to put other things in the stream. Version is now a property of the EditLogInputStream and retrieved using getVersion().

      1. HDFS-2187.diff
        17 kB
        Ivan Kelly
      2. HDFS-2187.diff
        30 kB
        Ivan Kelly
      3. 2187-changes.txt
        17 kB
        Todd Lipcon
      4. hdfs-2187.txt
        27 kB
        Todd Lipcon
      5. hdfs-2187.txt
        32 kB
        Todd Lipcon
      6. hdfs-2187.txt
        33 kB
        Todd Lipcon

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #812 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/812/)
          HDFS-2187. Make EditLogInputStream act like an iterator over FSEditLogOps. Contributed by Ivan Kelly and Todd Lipcon.

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

          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImageTransactionalStorageInspector.java
          • /hadoop/common/trunk/hdfs/CHANGES.txt
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFSEditLogLoader.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileOutputStream.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 #812 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/812/ ) HDFS-2187 . Make EditLogInputStream act like an iterator over FSEditLogOps. Contributed by Ivan Kelly and Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1153996 Files : /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImageTransactionalStorageInspector.java /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFSEditLogLoader.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileOutputStream.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 #738 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/738/)
          HDFS-2187. Make EditLogInputStream act like an iterator over FSEditLogOps. Contributed by Ivan Kelly and Todd Lipcon.

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

          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImageTransactionalStorageInspector.java
          • /hadoop/common/trunk/hdfs/CHANGES.txt
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFSEditLogLoader.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
          • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
          • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileOutputStream.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 #738 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/738/ ) HDFS-2187 . Make EditLogInputStream act like an iterator over FSEditLogOps. Contributed by Ivan Kelly and Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1153996 Files : /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImageTransactionalStorageInspector.java /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFSEditLogLoader.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileOutputStream.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/BackupImage.java
          Hide
          Todd Lipcon added a comment -

          Committed, thanks Ivan, as well as Jitendra for reviewing!

          Show
          Todd Lipcon added a comment - Committed, thanks Ivan, as well as Jitendra for reviewing!
          Hide
          Jitendra Nath Pandey added a comment -

          Patch looks good to me. +1

          Show
          Jitendra Nath Pandey added a comment - Patch looks good to me. +1
          Hide
          Todd Lipcon added a comment -

          Oops, Ivan just correctly pointed out that Holder.java has nothing to do with this patch. That's correct - it accidentally snuck in from something else in my working tree. New patch without that.

          Show
          Todd Lipcon added a comment - Oops, Ivan just correctly pointed out that Holder.java has nothing to do with this patch. That's correct - it accidentally snuck in from something else in my working tree. New patch without that.
          Hide
          Todd Lipcon added a comment -

          Made the change suggested by Ivan. Added license and javadoc to Holder.java.

          I ran test-patch locally and got all +1s. I also ran every unit test that has the word EditLog in it, and they passed.

          I'll commit this to trunk in a couple hours if there are no objections

          Show
          Todd Lipcon added a comment - Made the change suggested by Ivan. Added license and javadoc to Holder.java. I ran test-patch locally and got all +1s. I also ran every unit test that has the word EditLog in it, and they passed. I'll commit this to trunk in a couple hours if there are no objections
          Hide
          Todd Lipcon added a comment -

          OK, I'll move it to EditLogFileInputStream.validate(File f).

          Show
          Todd Lipcon added a comment - OK, I'll move it to EditLogFileInputStream.validate(File f).
          Hide
          Ivan Kelly added a comment -

          All look good except for the wrapper function for validateEditLog. I've no problem with the idea of it, but it is file specific in a class that knows nothing else about files. It belongs somewhere else, perhaps in the inspector, or maybe even on EditLogFileInputStream.

          Show
          Ivan Kelly added a comment - All look good except for the wrapper function for validateEditLog. I've no problem with the idea of it, but it is file specific in a class that knows nothing else about files. It belongs somewhere else, perhaps in the inspector, or maybe even on EditLogFileInputStream.
          Hide
          Todd Lipcon added a comment -

          Rather than write out separate comments, I just took Ivan's patch and ran with it a bit. Here are the changes I'm suggesting in the form of a diff:

          • make EditLogInputStream implement Closeable, so we can still use IOUtils.closeStream on it
          • make EditLogBackupInputStream.readOp() throw IllegalStateException instead of IOE if readOp is called without setting data first – this would be a coding error rather than something we expect to see
          • remove some redundant creation of new DataInputStreams
          • remove unused imports in EditLogFileInputStream
          • make EditLogFileInputStream members final
          • adds a new exception type in EditLogFileInputStream's constructor, so it's possible to differentiate between inability to open a file and an invalid header. The earlier iteration of the patch would end up storing logVersion as 0, and failing an assertion, I think.
          • made the validateEditLog call that takes a stream not close the stream when it finishes validation – generally I think it's best that whoever opens a stream be the one that closes it. Added a wrapper function that takes a File, and handles opening and closing the stream

          Are you guys OK with these changes? I attached both the delta and the new combined patch

          Show
          Todd Lipcon added a comment - Rather than write out separate comments, I just took Ivan's patch and ran with it a bit. Here are the changes I'm suggesting in the form of a diff: make EditLogInputStream implement Closeable, so we can still use IOUtils.closeStream on it make EditLogBackupInputStream.readOp() throw IllegalStateException instead of IOE if readOp is called without setting data first – this would be a coding error rather than something we expect to see remove some redundant creation of new DataInputStreams remove unused imports in EditLogFileInputStream make EditLogFileInputStream members final adds a new exception type in EditLogFileInputStream's constructor, so it's possible to differentiate between inability to open a file and an invalid header. The earlier iteration of the patch would end up storing logVersion as 0, and failing an assertion, I think. made the validateEditLog call that takes a stream not close the stream when it finishes validation – generally I think it's best that whoever opens a stream be the one that closes it. Added a wrapper function that takes a File, and handles opening and closing the stream Are you guys OK with these changes? I attached both the delta and the new combined patch
          Hide
          Ivan Kelly added a comment -

          Updated to apply on trunk.

          Show
          Ivan Kelly added a comment - Updated to apply on trunk.
          Hide
          Todd Lipcon added a comment -

          Looks like this doesn't apply against the post-merge trunk

          Show
          Todd Lipcon added a comment - Looks like this doesn't apply against the post-merge 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/12487443/HDFS-2187.diff
          against trunk revision 1149455.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

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

          -1 javac. The patch appears to cause tar ant target to fail.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

          +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:

          -1 contrib tests. The patch failed contrib unit tests.

          -1 system test framework. The patch failed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/994//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/994//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/12487443/HDFS-2187.diff against trunk revision 1149455. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +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: -1 contrib tests. The patch failed contrib unit tests. -1 system test framework. The patch failed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/994//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/994//console This message is automatically generated.
          Hide
          Ivan Kelly added a comment -

          Patch applies on top of HDFS-2149

          Show
          Ivan Kelly added a comment - Patch applies on top of HDFS-2149

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development