Details

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

      Description

      I plan to do some code refactoring to make HDFS-1070 simpler.

      1. refactorImageLoader.patch
        22 kB
        Hairong Kuang
      2. refactorImageLoader1.patch
        22 kB
        Hairong Kuang

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          Let's coordinate on this - I'm working on HDFS-1473 right now, and Ivan is working on HDFS-1489.

          If we all separately do refactorings it's going to be painful!

          Show
          Todd Lipcon added a comment - Let's coordinate on this - I'm working on HDFS-1473 right now, and Ivan is working on HDFS-1489 . If we all separately do refactorings it's going to be painful!
          Hide
          Jakob Homan added a comment -

          HDFS-1465 will hopefully be coming along too...

          Show
          Jakob Homan added a comment - HDFS-1465 will hopefully be coming along too...
          Hide
          Todd Lipcon added a comment -

          Youch. Looks like we have at least 4 parallel JIRAs Plus an email thread or two!

          How can we divide up this work effectively?

          Show
          Todd Lipcon added a comment - Youch. Looks like we have at least 4 parallel JIRAs Plus an email thread or two! How can we divide up this work effectively?
          Hide
          Hairong Kuang added a comment -

          Todd, as I discussed with you previously, please go ahead with HDFS-1473. I do not think this collides with HDFS-1489.

          Jakob, please let me know it goes with HDFS-1465 ...

          Show
          Hairong Kuang added a comment - Todd, as I discussed with you previously, please go ahead with HDFS-1473 . I do not think this collides with HDFS-1489 . Jakob, please let me know it goes with HDFS-1465 ...
          Hide
          Hairong Kuang added a comment -

          This patch does the following major changes:
          1. Add a method to load all the inodes;
          2. Add a method to load one inode except for its name;
          3. Make the ImageLoader constructor to take fsnamesystem as a parameter.

          Some subtle (and controversial) changes made are:
          1. When a child inode adds to namespace (fsdirectory), its parent inode is always known.
          2. When loading an inode, BlockInfos are created instead of Blocks.

          Show
          Hairong Kuang added a comment - This patch does the following major changes: 1. Add a method to load all the inodes; 2. Add a method to load one inode except for its name; 3. Make the ImageLoader constructor to take fsnamesystem as a parameter. Some subtle (and controversial) changes made are: 1. When a child inode adds to namespace (fsdirectory), its parent inode is always known. 2. When loading an inode, BlockInfos are created instead of Blocks.
          Hide
          Konstantin Boudnik added a comment -

          Hairong, just a quick look over the patch: JavaDoc for newINode doesn't have descriptions for the parameters. Perhaps, their names are self-explanatory?

          Show
          Konstantin Boudnik added a comment - Hairong, just a quick look over the patch: JavaDoc for newINode doesn't have descriptions for the parameters. Perhaps, their names are self-explanatory?
          Hide
          Todd Lipcon added a comment -

          Looks pretty good. Just some small nits and then +1 from my viewpoint.

          • FSNamesystem member in FSImageFormat.Loader should probably be marked final and maybe declared up with conf rather than down with the "output" members
          • The indentation is off on the loadFullNameINodes function
          • The indentation of the addToParent() function arguments is pretty strange looking
          • A few whitespace lines removed needlessly (before spaceConsumedInTree and following two functions)
          Show
          Todd Lipcon added a comment - Looks pretty good. Just some small nits and then +1 from my viewpoint. FSNamesystem member in FSImageFormat.Loader should probably be marked final and maybe declared up with conf rather than down with the "output" members The indentation is off on the loadFullNameINodes function The indentation of the addToParent() function arguments is pretty strange looking A few whitespace lines removed needlessly (before spaceConsumedInTree and following two functions)
          Hide
          Hairong Kuang added a comment -

          refactorImageLoader1.patch addressed Todd and Cos's review comments.

          Show
          Hairong Kuang added a comment - refactorImageLoader1.patch addressed Todd and Cos's review comments.
          Hide
          Todd Lipcon added a comment -

          +1 looks good to me assuming tests pass

          Show
          Todd Lipcon added a comment - +1 looks good to me assuming tests pass
          Hide
          Hairong Kuang added a comment -

          [exec] -1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] -1 tests included. The patch doesn't appear to include any new or modified tests.
          [exec] Please justify why no new tests are needed for this patch.
          [exec]
          [exec] Also please list what manual steps were performed to verify this patch.
          [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.

          No unit test is added since the patch mostly moves the code around.

          Show
          Hairong Kuang added a comment - [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] -1 tests included. The patch doesn't appear to include any new or modified tests. [exec] Please justify why no new tests are needed for this patch. [exec] [exec] Also please list what manual steps were performed to verify this patch. [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. No unit test is added since the patch mostly moves the code around.
          Hide
          Jakob Homan added a comment -

          +1. Looks good.

          Show
          Jakob Homan added a comment - +1. Looks good.
          Hide
          Hairong Kuang added a comment -

          Failed tests are all known:
          TestBlockRecovery
          TestHDFSTrash
          TestBackupNode
          TestStorageRestore
          TestBalancer
          TestBlockTokenWithDFS

          Show
          Hairong Kuang added a comment - Failed tests are all known: TestBlockRecovery TestHDFSTrash TestBackupNode TestStorageRestore TestBalancer TestBlockTokenWithDFS
          Hide
          Hairong Kuang added a comment -

          I've just committed this!

          Show
          Hairong Kuang added a comment - I've just committed this!
          Hide
          Hairong Kuang added a comment -

          I've committed this.

          Show
          Hairong Kuang added a comment - I've committed this.
          Hide
          Todd Lipcon added a comment -

          I'd like to propose this go into branch-0.22 as well, since we intend to put HDFS-1073 into 22, and all of those patches build on top of this. Would you mind committing if you don't object, Hairong?

          Show
          Todd Lipcon added a comment - I'd like to propose this go into branch-0.22 as well, since we intend to put HDFS-1073 into 22, and all of those patches build on top of this. Would you mind committing if you don't object, Hairong?
          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/ )

            People

            • Assignee:
              Hairong Kuang
              Reporter:
              Hairong Kuang
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development