Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-4304

Make FSEditLogOp.MAX_OP_SIZE configurable

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0, 2.0.3-alpha
    • Fix Version/s: 2.1.0-beta
    • Component/s: namenode
    • Labels:
      None

      Description

      Today we ran into an issue where a NN had logged a very large op, greater than the 1.5MB MAX_OP_SIZE constant. In order to successfully load the edits, we had to patch with a larger constant. This constant should be configurable so that we wouldn't have to recompile in these odd cases. Additionally, I think the default should be bumped a bit higher, since it's only a safeguard against OOME, and people tend to run NNs with multi-GB heaps.

      1. HDFS-4304.005.patch
        15 kB
        Colin Patrick McCabe
      2. HDFS-4304.004.patch
        15 kB
        Colin Patrick McCabe
      3. HDFS-4304.003.patch
        15 kB
        Colin Patrick McCabe
      4. HDFS-4304.002.patch
        15 kB
        Colin Patrick McCabe
      5. HDFS-4304.001.patch
        14 kB
        Colin Patrick McCabe

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        2h 25m 1 Colin Patrick McCabe 11/Dec/12 23:25
        Patch Available Patch Available Resolved Resolved
        72d 22h 3m 1 Aaron T. Myers 22/Feb/13 21:28
        Resolved Resolved Closed Closed
        186d 38m 1 Arun C Murthy 27/Aug/13 23:07
        Arun C Murthy made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #1353 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1353/)
        HDFS-4304. Make FSEditLogOp.MAX_OP_SIZE configurable. Contributed by Colin Patrick McCabe. (Revision 1449218)

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

        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/RedundantEditLogInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeRecovery.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1353 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1353/ ) HDFS-4304 . Make FSEditLogOp.MAX_OP_SIZE configurable. Contributed by Colin Patrick McCabe. (Revision 1449218) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1449218 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/RedundantEditLogInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeRecovery.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #1325 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1325/)
        HDFS-4304. Make FSEditLogOp.MAX_OP_SIZE configurable. Contributed by Colin Patrick McCabe. (Revision 1449218)

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

        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/RedundantEditLogInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeRecovery.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1325 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1325/ ) HDFS-4304 . Make FSEditLogOp.MAX_OP_SIZE configurable. Contributed by Colin Patrick McCabe. (Revision 1449218) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1449218 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/RedundantEditLogInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeRecovery.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Yarn-trunk #136 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/136/)
        HDFS-4304. Make FSEditLogOp.MAX_OP_SIZE configurable. Contributed by Colin Patrick McCabe. (Revision 1449218)

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

        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/RedundantEditLogInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeRecovery.java
        Show
        Hudson added a comment - Integrated in Hadoop-Yarn-trunk #136 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/136/ ) HDFS-4304 . Make FSEditLogOp.MAX_OP_SIZE configurable. Contributed by Colin Patrick McCabe. (Revision 1449218) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1449218 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/RedundantEditLogInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeRecovery.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-trunk-Commit #3377 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3377/)
        HDFS-4304. Make FSEditLogOp.MAX_OP_SIZE configurable. Contributed by Colin Patrick McCabe. (Revision 1449218)

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

        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/RedundantEditLogInputStream.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeRecovery.java
        Show
        Hudson added a comment - Integrated in Hadoop-trunk-Commit #3377 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3377/ ) HDFS-4304 . Make FSEditLogOp.MAX_OP_SIZE configurable. Contributed by Colin Patrick McCabe. (Revision 1449218) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1449218 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/RedundantEditLogInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeRecovery.java
        Aaron T. Myers made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags Reviewed [ 10343 ]
        Fix Version/s 2.0.4-beta [ 12324031 ]
        Resolution Fixed [ 1 ]
        Hide
        Aaron T. Myers added a comment -

        I've just committed this to trunk and branch-2.

        Thanks a lot for the contribution, Colin.

        Show
        Aaron T. Myers added a comment - I've just committed this to trunk and branch-2. Thanks a lot for the contribution, Colin.
        Aaron T. Myers made changes -
        Target Version/s 3.0.0, 2.0.3-alpha [ 12320356, 12323274 ] 3.0.0, 2.0.4-beta [ 12320356, 12324031 ]
        Aaron T. Myers made changes -
        Issue Type Bug [ 1 ] Improvement [ 4 ]
        Target Version/s 3.0.0, 2.0.3-alpha [ 12320356, 12323274 ] 2.0.3-alpha, 3.0.0 [ 12323274, 12320356 ]
        Hide
        Aaron T. Myers added a comment -

        Makes sense, Colin.

        +1, the latest patch looks good to me. I'm going to commit this momentarily.

        Show
        Aaron T. Myers added a comment - Makes sense, Colin. +1, the latest patch looks good to me. I'm going to commit this momentarily.
        Hide
        Colin Patrick McCabe added a comment -

        Last comment should read "the JN that is actually going to be writing the bytes to disk."

        Show
        Colin Patrick McCabe added a comment - Last comment should read "the JN that is actually going to be writing the bytes to disk."
        Colin Patrick McCabe made changes -
        Attachment HDFS-4304.005.patch [ 12563382 ]
        Hide
        Colin Patrick McCabe added a comment -

        This version of the patch makes MAX_OP_SIZE configurable in production and not just in recovery mode.

        I didn't implement the warning when writing an over-long opcode. It would be really tricky to do this "right"-- for example, if you're using QJM, the maximum op size on your local NameNode may not be the same as on the NN that is actually going to be writing the bytes to disk. I think that would get messy. This is just a minimal change to make something which wasn't configurable before, configurable.

        Show
        Colin Patrick McCabe added a comment - This version of the patch makes MAX_OP_SIZE configurable in production and not just in recovery mode. I didn't implement the warning when writing an over-long opcode. It would be really tricky to do this "right"-- for example, if you're using QJM, the maximum op size on your local NameNode may not be the same as on the NN that is actually going to be writing the bytes to disk. I think that would get messy. This is just a minimal change to make something which wasn't configurable before, configurable.
        Hide
        Aaron T. Myers added a comment -

        After thinking about this some more, I think we should make the config affect not just when running recovery mode, but also when just running normally. My reasoning is that if/when a user hits this issue, they'll want to just change the config and then restart their NN. Users might be confused that the setting doesn't apply to the normal edit log loading of the NN, and the user shouldn't have to run the edit logs through recovery mode first before starting their NN normally. This also implies that we should change the config property name not to include the word "recovery".

        Additionally, I really like this suggestion from Todd in a previous comment:

        I think it's better to write an edit which might result in a problem at next reboot than it is to actually crash a production cluster. Logging a WARN or ERROR is probably a great idea, though.

        Would you be up for adding such a log message in this patch as well?

        Show
        Aaron T. Myers added a comment - After thinking about this some more, I think we should make the config affect not just when running recovery mode, but also when just running normally. My reasoning is that if/when a user hits this issue, they'll want to just change the config and then restart their NN. Users might be confused that the setting doesn't apply to the normal edit log loading of the NN, and the user shouldn't have to run the edit logs through recovery mode first before starting their NN normally. This also implies that we should change the config property name not to include the word "recovery". Additionally, I really like this suggestion from Todd in a previous comment: I think it's better to write an edit which might result in a problem at next reboot than it is to actually crash a production cluster. Logging a WARN or ERROR is probably a great idea, though. Would you be up for adding such a log message in this patch as well?
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12560911/HDFS-4304.004.patch
        against trunk revision .

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

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

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

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

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

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal.

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

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3663//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3663//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/12560911/HDFS-4304.004.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3663//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3663//console This message is automatically generated.
        Hide
        Aaron T. Myers added a comment -

        Sorry, I wasn't clear. I intended to recommend you completely remove FSEditLogOp.DEFAULT_MAX_OP_SIZE and replace it with the default in DFSConfigKeys.

        Show
        Aaron T. Myers added a comment - Sorry, I wasn't clear. I intended to recommend you completely remove FSEditLogOp.DEFAULT_MAX_OP_SIZE and replace it with the default in DFSConfigKeys.
        Colin Patrick McCabe made changes -
        Attachment HDFS-4304.004.patch [ 12560911 ]
        Hide
        Colin Patrick McCabe added a comment -

        address aaron's suggestions

        Show
        Colin Patrick McCabe added a comment - address aaron's suggestions
        Hide
        Aaron T. Myers added a comment -

        Patch looks pretty good to me, except that I recommend you rename the configuration key variable to append "_KEY" and move the default value into DFSConfigKeys, as we do for most of the other config keys/default values.

        Show
        Aaron T. Myers added a comment - Patch looks pretty good to me, except that I recommend you rename the configuration key variable to append "_KEY" and move the default value into DFSConfigKeys, as we do for most of the other config keys/default values.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12560658/HDFS-4304.003.patch
        against trunk revision .

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

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

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

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

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

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal.

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

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3653//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3653//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/12560658/HDFS-4304.003.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3653//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3653//console This message is automatically generated.
        Colin Patrick McCabe made changes -
        Attachment HDFS-4304.003.patch [ 12560658 ]
        Hide
        Colin Patrick McCabe added a comment -

        I updated the comment to reflect the fact that opcode types are now cached in FSEditLogOp#Reader, not thread-local data.

        Show
        Colin Patrick McCabe added a comment - I updated the comment to reflect the fact that opcode types are now cached in FSEditLogOp#Reader , not thread-local data.
        Hide
        Colin Patrick McCabe added a comment -

        Would you elaborate on the reason why DEFAULT_MAX_OP_SIZE is increased to 50 MB?

        The rationale for increasing the default maximum op size is that OP_CLOSE contains a list of all blocks in the closed file. If we want to support really large files, we have to have a higher limit so that this OP_CLOSE does not end up being larger than the maximum edit log operation size. The comment in the code explains why 50 MB was chosen rather than something larger.

        Show
        Colin Patrick McCabe added a comment - Would you elaborate on the reason why DEFAULT_MAX_OP_SIZE is increased to 50 MB? The rationale for increasing the default maximum op size is that OP_CLOSE contains a list of all blocks in the closed file. If we want to support really large files, we have to have a higher limit so that this OP_CLOSE does not end up being larger than the maximum edit log operation size. The comment in the code explains why 50 MB was chosen rather than something larger.
        Hide
        Kihwal Lee added a comment -

        Would you elaborate on the reason why DEFAULT_MAX_OP_SIZE is increased to 50 MB?

        Show
        Kihwal Lee added a comment - Would you elaborate on the reason why DEFAULT_MAX_OP_SIZE is increased to 50 MB?
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12560480/HDFS-4304.002.patch
        against trunk revision .

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

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

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

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

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

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal.

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

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3644//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3644//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/12560480/HDFS-4304.002.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3644//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3644//console This message is automatically generated.
        Colin Patrick McCabe made changes -
        Attachment HDFS-4304.002.patch [ 12560480 ]
        Colin Patrick McCabe made changes -
        Attachment HDFS-4304.002.patch [ 12560479 ]
        Colin Patrick McCabe made changes -
        Attachment HDFS-4304.002.patch [ 12560479 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12560473/HDFS-4304.001.patch
        against trunk revision .

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

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

        -1 javac. The patch appears to cause the build to fail.

        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3642//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/12560473/HDFS-4304.001.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. -1 javac . The patch appears to cause the build to fail. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3642//console This message is automatically generated.
        Colin Patrick McCabe made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Colin Patrick McCabe added a comment -

        In addition to the unit test, I also tested:

        ./bin/hadoop namenode -Ddfs.namenode.recovery.max.op.size=1073741824 -recover
        
        Show
        Colin Patrick McCabe added a comment - In addition to the unit test, I also tested: ./bin/hadoop namenode -Ddfs.namenode.recovery.max.op.size=1073741824 -recover
        Hide
        Colin Patrick McCabe added a comment -

        Here's a patch which makes it configurable during recovery mode, and bumps the default up to 50MB.

        Show
        Colin Patrick McCabe added a comment - Here's a patch which makes it configurable during recovery mode, and bumps the default up to 50MB.
        Colin Patrick McCabe made changes -
        Field Original Value New Value
        Attachment HDFS-4304.001.patch [ 12560473 ]
        Hide
        Todd Lipcon added a comment -

        I am firmly of the opinion that all constants should be configurable, even if we can't see any reason to configure them. If this had been configurable, I would have been able to avoid shipping a custom patch build to a customer this morning – saving everyone time and effort.

        As for throwing an error when it's written – the problem is that the only way to throw an error mid-edit is to abort the NN, since the ops are written after they're applied to the in-memory state. I think it's better to write an edit which might result in a problem at next reboot than it is to actually crash a production cluster. Logging a WARN or ERROR is probably a great idea, though.

        Show
        Todd Lipcon added a comment - I am firmly of the opinion that all constants should be configurable, even if we can't see any reason to configure them. If this had been configurable, I would have been able to avoid shipping a custom patch build to a customer this morning – saving everyone time and effort. As for throwing an error when it's written – the problem is that the only way to throw an error mid-edit is to abort the NN, since the ops are written after they're applied to the in-memory state. I think it's better to write an edit which might result in a problem at next reboot than it is to actually crash a production cluster. Logging a WARN or ERROR is probably a great idea, though.
        Hide
        Colin Patrick McCabe added a comment -

        I don't think it should be configurable-- that way lies madness, as people mix and match configurations and get all kinds of loading errors and incompatibilities. I do think that it should be larger than 1.5 MB-- perhaps a factor of 50 or 100 larger. We should also fix the edit log output logic so that oversized ops cause an error when they are being written, not later down the line when they're being decoded.

        Show
        Colin Patrick McCabe added a comment - I don't think it should be configurable-- that way lies madness, as people mix and match configurations and get all kinds of loading errors and incompatibilities. I do think that it should be larger than 1.5 MB-- perhaps a factor of 50 or 100 larger. We should also fix the edit log output logic so that oversized ops cause an error when they are being written, not later down the line when they're being decoded.
        Todd Lipcon created issue -

          People

          • Assignee:
            Colin Patrick McCabe
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development