Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-563

Simplify the codes in FSNamesystem.getBlockLocations(..)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      There are un-used codes in FSNamesystem.getBlockLocations(..). Also, some codes can be moved to tests.

      1. h563_20090821.patch
        7 kB
        Tsz Wo Nicholas Sze
      2. h563_20090825.patch
        14 kB
        Tsz Wo Nicholas Sze
      3. h563_20090826.patch
        14 kB
        Tsz Wo Nicholas Sze

        Issue Links

          Activity

          Hide
          Tsz Wo Nicholas Sze added a comment -

          h563_20090821.patch: Simplify the codes in getBlockLocations(..).

          Show
          Tsz Wo Nicholas Sze added a comment - h563_20090821.patch: Simplify the codes in getBlockLocations(..).
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 9 new or modified tests.

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

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

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

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

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/80/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/80/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/80/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/80/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/12417337/h563_20090821.patch against trunk revision 806746. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/80/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/80/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/80/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/80/console This message is automatically generated.
          Hide
          Suresh Srinivas added a comment -

          I am not sure how useful the addition of Util.getBlockLocations() is. Other than that the patch looks good.

          Show
          Suresh Srinivas added a comment - I am not sure how useful the addition of Util.getBlockLocations() is. Other than that the patch looks good.
          Hide
          Konstantin Shvachko added a comment -

          This is a very good idea to expose namenode functionality for tests via a Util class rather than keeping public methods in the NameNode itself. Could you please add a JavaDoc comment saying something like "This is a utility class to expose NameNode functionality for unit tests."
          In order to establish the right precedence it would be better to change parameter FSNamesystem to NameNode in Util.getBlockLocations(). That way we will be able to make getNamesystem() method package private down the road.

          Show
          Konstantin Shvachko added a comment - This is a very good idea to expose namenode functionality for tests via a Util class rather than keeping public methods in the NameNode itself. Could you please add a JavaDoc comment saying something like "This is a utility class to expose NameNode functionality for unit tests." In order to establish the right precedence it would be better to change parameter FSNamesystem to NameNode in Util.getBlockLocations(). That way we will be able to make getNamesystem() method package private down the road.
          Hide
          Konstantin Boudnik added a comment -

          May I suggest to rename the class to something like AccessWrappers because it is likely that we are going to see more like these in the feature.

          Show
          Konstantin Boudnik added a comment - May I suggest to rename the class to something like AccessWrappers because it is likely that we are going to see more like these in the feature.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h563_20090825.patch:

          > I am not sure how useful the addition of Util.getBlockLocations() is. ...
          The idea is to remove some public methods which are used for unit tests.

          > In order to establish the right precedence it would be better to change parameter FSNamesystem to NameNode in Util.getBlockLocations(). That way we will be able to make getNamesystem() method package private down the road.
          Changed the parameter from FSNamesystem to NameNode. Also changed NameNode.getNamesystem() method package private.

          > May I suggest to rename the class to something like AccessWrappers because it is likely that we are going to see more like these in the feature.
          Renamed Util to AccessWrappers.

          Thanks Suresh, Konstantin and Cos for all the review comments.

          Show
          Tsz Wo Nicholas Sze added a comment - h563_20090825.patch: > I am not sure how useful the addition of Util.getBlockLocations() is. ... The idea is to remove some public methods which are used for unit tests. > In order to establish the right precedence it would be better to change parameter FSNamesystem to NameNode in Util.getBlockLocations(). That way we will be able to make getNamesystem() method package private down the road. Changed the parameter from FSNamesystem to NameNode. Also changed NameNode.getNamesystem() method package private. > May I suggest to rename the class to something like AccessWrappers because it is likely that we are going to see more like these in the feature. Renamed Util to AccessWrappers. Thanks Suresh, Konstantin and Cos for all the review comments.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 18 new or modified tests.

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

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

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

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

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/87/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/87/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/87/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/87/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/12417676/h563_20090825.patch against trunk revision 807818. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/87/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/87/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/87/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/87/console This message is automatically generated.
          Hide
          Konstantin Boudnik added a comment -

          The fallen test seems to be the direct result of HDFS-568

          Show
          Konstantin Boudnik added a comment - The fallen test seems to be the direct result of HDFS-568
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > The fallen test seems to be the direct result of HDFS-568
          Yes, the failed test is not related to this. It seems that build.xml has some problems. See this comment.

          Show
          Tsz Wo Nicholas Sze added a comment - > The fallen test seems to be the direct result of HDFS-568 Yes, the failed test is not related to this. It seems that build.xml has some problems. See this comment .
          Hide
          Konstantin Shvachko added a comment -

          I was thinking about the right name for this new class that provides public access to name-node's data for tests. Don't particularly like AccessWrappers because it also (as Util) obscures the meaning of the class.
          Came up with two names: NameNodeAdapter, NameNodeExplorer.

          Show
          Konstantin Shvachko added a comment - I was thinking about the right name for this new class that provides public access to name-node's data for tests. Don't particularly like AccessWrappers because it also (as Util) obscures the meaning of the class. Came up with two names: NameNodeAdapter, NameNodeExplorer.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h563_20090826.patch: renamed the class to NameNodeAdapter

          Show
          Tsz Wo Nicholas Sze added a comment - h563_20090826.patch: renamed the class to NameNodeAdapter
          Hide
          Konstantin Shvachko added a comment -

          +1

          Show
          Konstantin Shvachko added a comment - +1
          Hide
          Tsz Wo Nicholas Sze added a comment -

          The latest patch only changed a class name. So I will commit it without submitting to Hudson again.

          Show
          Tsz Wo Nicholas Sze added a comment - The latest patch only changed a class name. So I will commit it without submitting to Hudson again.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this.

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #65 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/65/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #65 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/65/ )

            People

            • Assignee:
              Tsz Wo Nicholas Sze
              Reporter:
              Tsz Wo Nicholas Sze
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development