Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-1073 Simpler model for Namenode's fs Image and edit Logs
  3. HDFS-1473

Refactor storage management into separate classes than fsimage file reading/writing

    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, 0.23.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Currently the FSImage class is responsible both for storage management (eg moving around files, tracking file names, the VERSION file, etc) as well as for the actual serialization and deserialization of the "fsimage" file within the storage directory.

      I'd like to refactor the loading and saving code into new classes. This will make testing easier and also make the major changes in HDFS-1073 easier to understand.

      1. hdfs-1473-prelim.txt
        73 kB
        Todd Lipcon
      2. hdfs-1473-followup.txt
        6 kB
        Todd Lipcon
      3. hdfs-1473-followup.3.txt
        9 kB
        Todd Lipcon
      4. hdfs-1473-followup.2.txt
        9 kB
        Todd Lipcon
      5. hdfs-1473.txt
        74 kB
        Todd Lipcon
      6. hdfs-1473.txt
        0.6 kB
        Todd Lipcon
      7. hdfs-1473.txt
        75 kB
        Todd Lipcon

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          Here's a super-preliminary patch for this. I moved around the bulk of the stuff that had to move, but there are still some things that need some real cleanup plus new javadoc. For example, there are currently 6 constructors used for FSImage which get pretty messy, since we now have a conf member.

          Just wanted to upload the patch to get comments if the general division of classes seems reasonable

          Show
          Todd Lipcon added a comment - Here's a super-preliminary patch for this. I moved around the bulk of the stuff that had to move, but there are still some things that need some real cleanup plus new javadoc. For example, there are currently 6 constructors used for FSImage which get pretty messy, since we now have a conf member. Just wanted to upload the patch to get comments if the general division of classes seems reasonable
          Hide
          Hairong Kuang added a comment -

          In general, it looks good.
          1. better merge ImageLoader & ImageWriter into one class because they both work on the same format.
          2. could ImageLoader/Writer has namesystem as a field?

          Show
          Hairong Kuang added a comment - In general, it looks good. 1. better merge ImageLoader & ImageWriter into one class because they both work on the same format. 2. could ImageLoader/Writer has namesystem as a field?
          Hide
          Todd Lipcon added a comment -

          How about FSImageFormat, with two inner static classes, Loader and Writer? I like keeping them as "one-shot" classes where the member variables are set by the loading/writing and then can be fetched. This makes it easy to understand which members are results of writing vs results of reading.

          Show
          Todd Lipcon added a comment - How about FSImageFormat, with two inner static classes, Loader and Writer? I like keeping them as "one-shot" classes where the member variables are set by the loading/writing and then can be fetched. This makes it easy to understand which members are results of writing vs results of reading.
          Hide
          Hairong Kuang added a comment -

          Sounds good to me. Personally I feel that it is simpler without the inner classes. But it is your call.

          Show
          Hairong Kuang added a comment - Sounds good to me. Personally I feel that it is simpler without the inner classes. But it is your call.
          Hide
          Todd Lipcon added a comment -

          Here's a patch that's been cleaned up and javadocced. I took Hairong's suggestion to combine the loader and saver as inner classes in an FSImageFormat class.

          As for making the FSNamesystem a member of that class instead of a parameter, it doesn't seem to have an advantage or disadvantage, so left as is. I think as we clean this up more with the other ongoing refactors it'll become clear which of the two is better and we can make the change then.

          We can continue to improve on this, but since these refactors cause lots of conflicts, I think it's best to treat this as "final" for this issue and then do some more improvements in followup commits. As is, I think it's better than it was before

          Show
          Todd Lipcon added a comment - Here's a patch that's been cleaned up and javadocced. I took Hairong's suggestion to combine the loader and saver as inner classes in an FSImageFormat class. As for making the FSNamesystem a member of that class instead of a parameter, it doesn't seem to have an advantage or disadvantage, so left as is. I think as we clean this up more with the other ongoing refactors it'll become clear which of the two is better and we can make the change then. We can continue to improve on this, but since these refactors cause lots of conflicts, I think it's best to treat this as "final" for this issue and then do some more improvements in followup commits. As is, I think it's better than it was before
          Hide
          Todd Lipcon added a comment -

          Unit test results:
          [junit] Test org.apache.hadoop.hdfs.TestHDFSTrash FAILED (timeout) [ HDFS-1471]
          [junit] Test org.apache.hadoop.hdfs.server.namenode.TestStorageRestore FAILED HDFS-1496
          [junit] Test org.apache.hadoop.hdfs.server.balancer.TestBalancer FAILED HDFS-613
          [junit] Test org.apache.hadoop.hdfs.server.namenode.TestBlockTokenWithDFS FAILED HDFS-613
          [junit] Test org.apache.hadoop.hdfs.server.namenode.TestSaveNamespace FAILED HDFS-1503
          [junit] Test org.apache.hadoop.hdfs.server.datanode.TestBlockRecovery FAILED HDFS-1502

          test-patch had one new findbugs issue, running through a slightly updated patch now.

          Show
          Todd Lipcon added a comment - Unit test results: [junit] Test org.apache.hadoop.hdfs.TestHDFSTrash FAILED (timeout) [ HDFS-1471 ] [junit] Test org.apache.hadoop.hdfs.server.namenode.TestStorageRestore FAILED HDFS-1496 [junit] Test org.apache.hadoop.hdfs.server.balancer.TestBalancer FAILED HDFS-613 [junit] Test org.apache.hadoop.hdfs.server.namenode.TestBlockTokenWithDFS FAILED HDFS-613 [junit] Test org.apache.hadoop.hdfs.server.namenode.TestSaveNamespace FAILED HDFS-1503 [junit] Test org.apache.hadoop.hdfs.server.datanode.TestBlockRecovery FAILED HDFS-1502 test-patch had one new findbugs issue, running through a slightly updated patch now.
          Hide
          Todd Lipcon added a comment -

          Here's the same patch plus a findbugs exclusion. For whatever reason, findbugs seems to think that the finally

          { out.close() }

          in the FSImage write function won't close the underlying streams. Since it's a FilterOutputStream, it does pass the close() down.

          The unit test results are same as posted above. Here's test-patch:

          [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 3 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.9) 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.

          Show
          Todd Lipcon added a comment - Here's the same patch plus a findbugs exclusion. For whatever reason, findbugs seems to think that the finally { out.close() } in the FSImage write function won't close the underlying streams. Since it's a FilterOutputStream, it does pass the close() down. The unit test results are same as posted above. Here's test-patch: [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 3 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.9) 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.
          Hide
          Todd Lipcon added a comment -

          Accidentally just attached the delta - this is the full patch.

          Show
          Todd Lipcon added a comment - Accidentally just attached the delta - this is the full patch.
          Hide
          Ivan Kelly added a comment -

          Looks good.

          +1

          Show
          Ivan Kelly added a comment - Looks good. +1
          Hide
          Eli Collins added a comment -

          Hey Todd,

          Looks great.

          Why do we no longer assert the return value of getLayoutVersion() is negative in loadFSImage? This can be handled in a follow on change, if necessary.

          Mind filing a jira for the cleanup you suggest in the TODO in the FSImage contructor?

          Thanks,
          Eli

          Show
          Eli Collins added a comment - Hey Todd, Looks great. Why do we no longer assert the return value of getLayoutVersion() is negative in loadFSImage? This can be handled in a follow on change, if necessary. Mind filing a jira for the cleanup you suggest in the TODO in the FSImage contructor? Thanks, Eli
          Hide
          Eli Collins added a comment -

          +1

          I'll commit this to trunk and branch 22 to minimize deltas since HDFS-1073 and other patches for 22 depend on this.

          Show
          Eli Collins added a comment - +1 I'll commit this to trunk and branch 22 to minimize deltas since HDFS-1073 and other patches for 22 depend on this.
          Hide
          Eli Collins added a comment -

          Thanks Todd!

          Show
          Eli Collins added a comment - Thanks Todd!
          Hide
          Todd Lipcon added a comment -

          Hey Eli, Looks like the new files in the patch weren't included in the svn commit:

          :000000 100644 0000000... 0d96937... A src/java/org/apache/hadoop/hdfs/server/namenode/FSImageCompression.java
          :000000 100644 0000000... 2db748a... A src/java/org/apache/hadoop/hdfs/server/namenode/FSImageFormat.java
          :000000 100644 0000000... 08ead09... A src/java/org/apache/hadoop/hdfs/server/namenode/FSImageSerialization.java

          Show
          Todd Lipcon added a comment - Hey Eli, Looks like the new files in the patch weren't included in the svn commit: :000000 100644 0000000... 0d96937... A src/java/org/apache/hadoop/hdfs/server/namenode/FSImageCompression.java :000000 100644 0000000... 2db748a... A src/java/org/apache/hadoop/hdfs/server/namenode/FSImageFormat.java :000000 100644 0000000... 08ead09... A src/java/org/apache/hadoop/hdfs/server/namenode/FSImageSerialization.java
          Hide
          Eli Collins added a comment -

          Added. Sorry bout that.

          Show
          Eli Collins added a comment - Added. Sorry bout that.
          Hide
          Konstantin Shvachko added a comment -

          Sorry for late response. Was just looking at it on Sunday when Eli committed it, so I stopped. Now looking at it in trunk I feel it needs more work, mostly because I see lots of unnecessary public methods and classes.

          1. FSImageCompression should not have public methods except for toString().
          2. FSImageFormat should not be abstract and should not have anything public.
          3. FSImageSerialization should not be abstract. It does not have any abstract methods. I understand we will get rid of other public qualifiers once OIV moves inside namenode package. Could we please state it in the beginning of the class.
          4. FSImageFormat.Loader and FSImageFormat.Writer seem like a mismatch. Loader rhythms with saver, reader with writer. Just a teerminology issue. We used to load and save image. Now we load and write it. I'd stay with the traditional terms.
          5. FSImageSerialization.TL_DATA is the new name for former FSImage.FILE_PERM. I agree FILE_PERM is not the best choice, but TL_DATA lacks any meaning attributed to the object. And if it once will become a non-thread local object then the name will be completely irrelevant. I'd go with the original name and rename in a separate issue. In general, refactoring patches should avoid renaming.
          Show
          Konstantin Shvachko added a comment - Sorry for late response. Was just looking at it on Sunday when Eli committed it, so I stopped. Now looking at it in trunk I feel it needs more work, mostly because I see lots of unnecessary public methods and classes. FSImageCompression should not have public methods except for toString(). FSImageFormat should not be abstract and should not have anything public. FSImageSerialization should not be abstract. It does not have any abstract methods. I understand we will get rid of other public qualifiers once OIV moves inside namenode package. Could we please state it in the beginning of the class. FSImageFormat.Loader and FSImageFormat.Writer seem like a mismatch. Loader rhythms with saver, reader with writer. Just a teerminology issue. We used to load and save image. Now we load and write it. I'd stay with the traditional terms. FSImageSerialization.TL_DATA is the new name for former FSImage.FILE_PERM. I agree FILE_PERM is not the best choice, but TL_DATA lacks any meaning attributed to the object. And if it once will become a non-thread local object then the name will be completely irrelevant. I'd go with the original name and rename in a separate issue. In general, refactoring patches should avoid renaming.
          Hide
          Todd Lipcon added a comment -

          Hi Konstantin. Thanks for the review. I fixed points 1 through 4 but disagree on point 5. Will upload the patch momentarily.

          FSImageSerialization.TL_DATA is the new name for former FSImage.FILE_PERM

          This isn't quite true. TL_DATA is now a struct which includes both FILE_PERM and the static U_STR object which used to be incorrectly non-threadlocal. Sorry to have included the bug fix in the refactor patch, but while I was moving U_STR anyway I figured it was a good idea to address the issue.

          So, TlData/TL_DATA themselves are descriptive of what they are - simply a container for thread-local data. The members of this struct are named as they were before. If it weren't thread local we wouldn't need this struct at all, and it would cease to exist.

          Show
          Todd Lipcon added a comment - Hi Konstantin. Thanks for the review. I fixed points 1 through 4 but disagree on point 5. Will upload the patch momentarily. FSImageSerialization.TL_DATA is the new name for former FSImage.FILE_PERM This isn't quite true. TL_DATA is now a struct which includes both FILE_PERM and the static U_STR object which used to be incorrectly non-threadlocal. Sorry to have included the bug fix in the refactor patch, but while I was moving U_STR anyway I figured it was a good idea to address the issue. So, TlData/TL_DATA themselves are descriptive of what they are - simply a container for thread-local data. The members of this struct are named as they were before. If it weren't thread local we wouldn't need this struct at all, and it would cease to exist.
          Hide
          Konstantin Shvachko added a comment -

          The Saver should save() rather than write(), the same way as Loader loads.
          Other than that it looks good.
          Good catch that U_STR has not been thread local. HDFS-1071 should have done that. Does that mean there is no test cases that save in parallel? That should fail when we save FileUnderConstruction.

          Show
          Konstantin Shvachko added a comment - The Saver should save() rather than write(), the same way as Loader loads. Other than that it looks good. Good catch that U_STR has not been thread local. HDFS-1071 should have done that. Does that mean there is no test cases that save in parallel? That should fail when we save FileUnderConstruction.
          Hide
          Todd Lipcon added a comment -

          I'll fix the save() -> write() momentarily.

          As for the test case, there are some tests that save in parallel, but I think the race is really narrow. U_STR is only used very briefly before it's written out to disk, so the likelihood of threads contending is probably pretty low. The images we write in the test cases also aren't huge - I'm more surprised that FB hasn't seen this yet on their large image tests. Hairong/Dmytro?

          Show
          Todd Lipcon added a comment - I'll fix the save() -> write() momentarily. As for the test case, there are some tests that save in parallel, but I think the race is really narrow. U_STR is only used very briefly before it's written out to disk, so the likelihood of threads contending is probably pretty low. The images we write in the test cases also aren't huge - I'm more surprised that FB hasn't seen this yet on their large image tests. Hairong/Dmytro?
          Hide
          Todd Lipcon added a comment -

          New patch fixes write->save, written->saved, etc.

          Show
          Todd Lipcon added a comment - New patch fixes write->save, written->saved, etc.
          Hide
          Todd Lipcon added a comment -

          Of course the patch went out of date minutes after it was posted... rebased patch on trunk. Konstantin, can you commit this if it looks good? Thanks.

          Show
          Todd Lipcon added a comment - Of course the patch went out of date minutes after it was posted... rebased patch on trunk. Konstantin, can you commit this if it looks good? Thanks.
          Hide
          Konstantin Shvachko added a comment -

          Could you please run tests or post results if you did. I'll commit it then.

          Show
          Konstantin Shvachko added a comment - Could you please run tests or post results if you did. I'll commit it then.
          Hide
          Todd Lipcon added a comment -

          Since the patch is just renames, I didn't run the full tests. I did verify that it compiles and passes commit tests which seems sufficient

          Show
          Todd Lipcon added a comment - Since the patch is just renames, I didn't run the full tests. I did verify that it compiles and passes commit tests which seems sufficient
          Hide
          Konstantin Shvachko added a comment -

          I just committed this. Thank you Todd.

          Show
          Konstantin Shvachko added a comment - I just committed this. Thank you Todd.
          Hide
          Hudson added a comment -

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

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

          Integrated in Hadoop-Hdfs-22-branch #35 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-22-branch/35/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-22-branch #35 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-22-branch/35/ )

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development