Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.22.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Before the major work in HDFS-1073, I'd like to do this refactor to clean up the monster FSEditLog class. We can separate all the functions that take of loading edits into an FSN from the functions that take care of writing edits, rolling, etc.

      1. hdfs-1462.txt
        68 kB
        Todd Lipcon
      2. hdfs-1462.txt
        68 kB
        Todd Lipcon
      3. hdfs-1462.txt
        68 kB
        Todd Lipcon
      4. hdfs-1462.txt
        75 kB
        Todd Lipcon
      5. hdfs-1462.txt
        75 kB
        Eli Collins

        Activity

        Hide
        Todd Lipcon added a comment -

        This patch is basically a straight refactor. It moves all of the edits-loading based stuff out of FSEditLog so we can separate the more stateful writing code from the less stateful reading code.

        Running through an internal test queue now.

        Show
        Todd Lipcon added a comment - This patch is basically a straight refactor. It moves all of the edits-loading based stuff out of FSEditLog so we can separate the more stateful writing code from the less stateful reading code. Running through an internal test queue now.
        Hide
        Todd Lipcon added a comment -

        Ran tests on this, got the following failures which also exist on trunk (filed JIRAs for all of these, most of which were previously reported in the comments on HDFS-96):
        [junit] Test org.apache.hadoop.hdfs.TestFileStatus FAILED (HDFS-1470)
        [junit] Test org.apache.hadoop.hdfs.TestHDFSTrash FAILED (timeout) (HDFS-1471)
        [junit] Test org.apache.hadoop.fs.TestFcHdfsSymlink FAILED (HDFS-1466)
        [junit] Test org.apache.hadoop.fs.TestHDFSFileContextMainOperations FAILED (HDFS-1466)
        [junit] Test org.apache.hadoop.hdfs.TestPipelines FAILED (HDFS-1467)
        [junit] Test org.apache.hadoop.hdfs.server.datanode.TestBlockReport FAILED (HDFS-1468)
        [junit] Test org.apache.hadoop.hdfs.server.namenode.TestBlockTokenWithDFS FAILED (HDFS-1469)

        This failed on the previous patch:
        [junit] Test org.apache.hadoop.hdfs.server.namenode.TestBackupNode FAILED (timeout)
        with a somewhat obvious NPE. This new version of the patch addresses the issue and the test now passes.

        Show
        Todd Lipcon added a comment - Ran tests on this, got the following failures which also exist on trunk (filed JIRAs for all of these, most of which were previously reported in the comments on HDFS-96 ): [junit] Test org.apache.hadoop.hdfs.TestFileStatus FAILED ( HDFS-1470 ) [junit] Test org.apache.hadoop.hdfs.TestHDFSTrash FAILED (timeout) ( HDFS-1471 ) [junit] Test org.apache.hadoop.fs.TestFcHdfsSymlink FAILED ( HDFS-1466 ) [junit] Test org.apache.hadoop.fs.TestHDFSFileContextMainOperations FAILED ( HDFS-1466 ) [junit] Test org.apache.hadoop.hdfs.TestPipelines FAILED ( HDFS-1467 ) [junit] Test org.apache.hadoop.hdfs.server.datanode.TestBlockReport FAILED ( HDFS-1468 ) [junit] Test org.apache.hadoop.hdfs.server.namenode.TestBlockTokenWithDFS FAILED ( HDFS-1469 ) This failed on the previous patch: [junit] Test org.apache.hadoop.hdfs.server.namenode.TestBackupNode FAILED (timeout) with a somewhat obvious NPE. This new version of the patch addresses the issue and the test now passes.
        Hide
        Todd Lipcon added a comment -

        Ran test-patch, got:

        [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 9 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 appears to introduce 1 new Findbugs warnings.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        [exec]
        [exec] +1 system tests framework. The patch passed system tests framework compile.

        The new findbugs warning is:

        Should org.apache.hadoop.hdfs.server.namenode.FSEditLog$Ops be a static inner class?

        which seems reasonable enough. New patch fixes this - test-patch is all +1 on this version. Should be good to go pending review.

        Show
        Todd Lipcon added a comment - Ran test-patch, got: [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 9 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 appears to introduce 1 new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] +1 system tests framework. The patch passed system tests framework compile. The new findbugs warning is: Should org.apache.hadoop.hdfs.server.namenode.FSEditLog$Ops be a static inner class? which seems reasonable enough. New patch fixes this - test-patch is all +1 on this version. Should be good to go pending review.
        Hide
        Jitendra Nath Pandey added a comment -

        This patch removes the references to namesystem object from FSEditlog, which is a good thing.
        FSEditlog still refers to FSNamesystem.LOG, if LOG object could be passed as a parameter, we could remove all references to FSNamesystem from FSEditlog.

        Show
        Jitendra Nath Pandey added a comment - This patch removes the references to namesystem object from FSEditlog, which is a good thing. FSEditlog still refers to FSNamesystem.LOG, if LOG object could be passed as a parameter, we could remove all references to FSNamesystem from FSEditlog.
        Hide
        Todd Lipcon added a comment -

        Sure, we could pass in LOG, or just create our own logger for FSEditLog. I left it as is to make it a straight refactor, but so long as people don't mind the slightly "breaking change" of changing the log category, I'm fine changing it. Thoughts?

        Show
        Todd Lipcon added a comment - Sure, we could pass in LOG, or just create our own logger for FSEditLog. I left it as is to make it a straight refactor, but so long as people don't mind the slightly "breaking change" of changing the log category, I'm fine changing it. Thoughts?
        Hide
        Sanjay Radia added a comment -

        +1 Looks good.
        Not worth changing the Log category - just pass as parameter, This leaves it as a
        straight refactor and at the same time removes all references to FSNamesystem from FSEditlog.

        Show
        Sanjay Radia added a comment - +1 Looks good. Not worth changing the Log category - just pass as parameter, This leaves it as a straight refactor and at the same time removes all references to FSNamesystem from FSEditlog.
        Hide
        Todd Lipcon added a comment -

        Looks like FSEditLog refers to FSImage.LOG in a couple of places. Should I leave those, or have them use the passed log?

        Show
        Todd Lipcon added a comment - Looks like FSEditLog refers to FSImage.LOG in a couple of places. Should I leave those, or have them use the passed log?
        Hide
        Sanjay Radia added a comment -

        Had not realized that there is already a log category for FSImage.
        Given that, don't pass the FSNamesystem.LOG as a parameter, create one for the FSEditLog and use it.

        Show
        Sanjay Radia added a comment - Had not realized that there is already a log category for FSImage. Given that, don't pass the FSNamesystem.LOG as a parameter, create one for the FSEditLog and use it.
        Hide
        Todd Lipcon added a comment -

        New patch moves all logs in FSEditLog.java to its own log instance. I also removed the following log message:

        -        if(FSImage.LOG.isDebugEnabled()) {
        -          FSImage.LOG.debug("loggin edits into " + eStream.getName() +
        -              " stream");
        -        }
        

        which was added post 0.20 and seems way too verbose to be useful (logs once per edit per log stream!). Hope that's OK.

        Show
        Todd Lipcon added a comment - New patch moves all logs in FSEditLog.java to its own log instance. I also removed the following log message: - if (FSImage.LOG.isDebugEnabled()) { - FSImage.LOG.debug( "loggin edits into " + eStream.getName() + - " stream" ); - } which was added post 0.20 and seems way too verbose to be useful (logs once per edit per log stream!). Hope that's OK.
        Hide
        Eli Collins added a comment -

        +1

        Nit: In FSNamesytem#adjustReplication typo "replicatoin" And please add spacing around operators.

        Otherwise looks great. Agree with removing the last log. This was added in HADOOP-4045, I checked that change and didn't find any other logs that fire with every edit.

        Show
        Eli Collins added a comment - +1 Nit: In FSNamesytem#adjustReplication typo "replicatoin" And please add spacing around operators. Otherwise looks great. Agree with removing the last log. This was added in HADOOP-4045 , I checked that change and didn't find any other logs that fire with every edit.
        Hide
        Eli Collins added a comment -

        Latest patch adjusted to fix the typo/spacing.

        Show
        Eli Collins added a comment - Latest patch adjusted to fix the typo/spacing.
        Hide
        Eli Collins added a comment -

        I've committed this. Thanks Todd.

        Test results on the latest patch:

             [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 9 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 warnings.
             [exec] 
             [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
             [exec] 
             [exec]     +1 system tests framework.  The patch passed system tests framework compile.
             [exec] 
        
        Show
        Eli Collins added a comment - I've committed this. Thanks Todd. Test results on the latest patch: [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 9 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 warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] +1 system tests framework. The patch passed system tests framework compile. [exec]
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #420 (See https://hudson.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/420/)
        Add missing file in last commit (HDFS-1462).

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #420 (See https://hudson.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/420/ ) Add missing file in last commit ( HDFS-1462 ).

          People

          • Assignee:
            Todd Lipcon
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development