Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-705

Create an adapter to access some of package-private methods of DataNode from tests

    Details

    • Type: Test Test
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.21.0
    • Component/s: test
    • Labels:
      None

      Description

      Some of tests from packages other than org.apache.hadoop.hdfs.server.datanode need an access to package-private methods of that package. Proposed patch will add new adapter class which will have public getter methods to provide such access to outside of the package.

      This is TEST ONLY class: it never has to be used for ANY development purposes.

      1. HDFS-705.patch
        2 kB
        Konstantin Boudnik
      2. HDFS-705.patch
        2 kB
        Konstantin Boudnik
      3. HDFS-705.patch
        2 kB
        Konstantin Boudnik

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          22h 30m 1 Konstantin Boudnik 15/Oct/09 21:31
          Resolved Resolved Closed Closed
          313d 18m 1 Tom White 24/Aug/10 21:49
          Tom White made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Tom White made changes -
          Fix Version/s 0.21.0 [ 12314046 ]
          Fix Version/s 0.22.0 [ 12314241 ]
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #78 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/78/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #78 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/78/ )
          Hide
          Hudson added a comment -

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

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

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

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

          Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #47 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/47/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #47 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/47/ )
          Konstantin Boudnik made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Konstantin Boudnik added a comment -

          I've just committed this

          Show
          Konstantin Boudnik added a comment - I've just committed this
          Hide
          Konstantin Boudnik added a comment -

          Oops, seems like I forgot to post the results of test-patch run. Here it is

          There appear to be 109 release audit warnings before the patch and 109 release audit warnings after applying the patch.
          
          +1 overall.  
              +1 @author.  The patch does not contain any @author tags.
              +1 tests included.  The patch appears to include 3 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.
          
          Show
          Konstantin Boudnik added a comment - Oops, seems like I forgot to post the results of test-patch run. Here it is There appear to be 109 release audit warnings before the patch and 109 release audit warnings after applying the patch. +1 overall. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 patch looks good

          Show
          Tsz Wo Nicholas Sze added a comment - +1 patch looks good
          Konstantin Boudnik made changes -
          Attachment HDFS-705.patch [ 12422170 ]
          Hide
          Konstantin Boudnik added a comment -

          I agree with Konstantin's point: we need to move test related methods to an adapters like this one wherever possible. Your comments are addressed (I have no idea why I was dissecting this poor thing instead of calling getSimplename() - don't ask

          On the second thought (thanks Nicholas) i decided to get rid of the assertion because it will add hardcoded limitation and might affect tests which are coming from differently named classes. Although the chances are slim.

          Thus, simplified version of the patch, but essentially the same.

          Show
          Konstantin Boudnik added a comment - I agree with Konstantin's point: we need to move test related methods to an adapters like this one wherever possible. Your comments are addressed (I have no idea why I was dissecting this poor thing instead of calling getSimplename() - don't ask On the second thought (thanks Nicholas) i decided to get rid of the assertion because it will add hardcoded limitation and might affect tests which are coming from differently named classes. Although the chances are slim. Thus, simplified version of the patch, but essentially the same.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > In fact, I thought that it might be pretty cool idea to assert on invocation class' name if it doesn't start with Test...

          We only have to make sure the source codes can be compiled without the test codes. Check name may not help much.

          Show
          Tsz Wo Nicholas Sze added a comment - > In fact, I thought that it might be pretty cool idea to assert on invocation class' name if it doesn't start with Test... We only have to make sure the source codes can be compiled without the test codes. Check name may not help much.
          Hide
          Konstantin Shvachko added a comment -

          I think it is a good idea to combine all test tools in test directories. This was started with NameNodeAdapter. This is another adapter for exposing data-node internals for testing. I think it is better when classes like DataNode or NameNode contain the essential logic only not polluted with public methods for testing whether they are marked as such with annotations or naming conventions.
          We should move all testing methods from DataNode and NameNode to the respective adapters,
          and stop accessing volumeMap, blockManager, and alike directly from data-node and name-node tests.

          1. TestNNLeaseRecovery is not yet in the code base, and probably does not need to be imported.
          2. Instead of using getClassName() and then looking for a substring you can call getClass().getSimpleName(), which will return the class name without the package.

          The rest look good to me +1.

          Show
          Konstantin Shvachko added a comment - I think it is a good idea to combine all test tools in test directories. This was started with NameNodeAdapter. This is another adapter for exposing data-node internals for testing. I think it is better when classes like DataNode or NameNode contain the essential logic only not polluted with public methods for testing whether they are marked as such with annotations or naming conventions. We should move all testing methods from DataNode and NameNode to the respective adapters, and stop accessing volumeMap , blockManager , and alike directly from data-node and name-node tests. TestNNLeaseRecovery is not yet in the code base, and probably does not need to be imported. Instead of using getClassName() and then looking for a substring you can call getClass().getSimpleName() , which will return the class name without the package. The rest look good to me +1.
          Hide
          Philip Zeyliger added a comment -

          In fact, I thought that it might be pretty cool idea to assert on invocation class' name if it doesn't start with Test...

          It's cute, but it seems unnecessary. If people are loading classes from foo-test.jar, they deserve everything that goes wrong.

          In a previous life, I simply stuck a VisibleForTesting annotation on things (http://code.google.com/p/google-collections/source/browse/trunk/src/com/google/common/annotations/VisibleForTesting.java?spec=svn80&r=80) and marked the method I needed public.

          Show
          Philip Zeyliger added a comment - In fact, I thought that it might be pretty cool idea to assert on invocation class' name if it doesn't start with Test... It's cute, but it seems unnecessary. If people are loading classes from foo-test.jar, they deserve everything that goes wrong. In a previous life, I simply stuck a VisibleForTesting annotation on things ( http://code.google.com/p/google-collections/source/browse/trunk/src/com/google/common/annotations/VisibleForTesting.java?spec=svn80&r=80 ) and marked the method I needed public.
          Hide
          Aaron Kimball added a comment -

          As a contrasting datapoint, in my own code I often get around this issue by creating public accessor methods which are named e.g. "getFooForTestingOnly()". So it's pretty clear that if you're using this, that it's an API not intended for stable use.

          Show
          Aaron Kimball added a comment - As a contrasting datapoint, in my own code I often get around this issue by creating public accessor methods which are named e.g. "getFooForTestingOnly()". So it's pretty clear that if you're using this, that it's an API not intended for stable use.
          Konstantin Boudnik made changes -
          Attachment HDFS-705.patch [ 12422161 ]
          Hide
          Konstantin Boudnik added a comment -

          In fact, I thought that it might be pretty cool idea to assert on invocation class' name if it doesn't start with Test...

          Show
          Konstantin Boudnik added a comment - In fact, I thought that it might be pretty cool idea to assert on invocation class' name if it doesn't start with Test...
          Hide
          Philip Zeyliger added a comment -

          Ok, thanks for the explanation.

          Show
          Philip Zeyliger added a comment - Ok, thanks for the explanation.
          Hide
          Konstantin Boudnik added a comment -

          Also, having access methods like this one has yet another advantage. One can assert if the invocation hasn't happened from a class which, say, starts with 'Test' prefix. Pretty efficient safeguard, I'd say

          Show
          Konstantin Boudnik added a comment - Also, having access methods like this one has yet another advantage. One can assert if the invocation hasn't happened from a class which, say, starts with 'Test' prefix. Pretty efficient safeguard, I'd say
          Hide
          Konstantin Boudnik added a comment -

          It is totally internal to HDFS. However, I'm working on a test which has to be in server.namenode package but is needed an access to a block's replica info (which is accessible through a server.datanode call).

          Also, this new getter is kept withing tests' source code and isn't a part of application source code. I'm opposing any public API's put into the production code merely for the testing purposes: it's bad. In fact, this was one of the reasons to have injection framework in Hadoop (HADOOP-6204, HDFS-435)

          JSR 294 might help address the issue. However, the last time I checked it was in 'Inactive' state, so...

          Show
          Konstantin Boudnik added a comment - It is totally internal to HDFS. However, I'm working on a test which has to be in server.namenode package but is needed an access to a block's replica info (which is accessible through a server.datanode call). Also, this new getter is kept withing tests' source code and isn't a part of application source code. I'm opposing any public API's put into the production code merely for the testing purposes: it's bad. In fact, this was one of the reasons to have injection framework in Hadoop ( HADOOP-6204 , HDFS-435 ) JSR 294 might help address the issue. However, the last time I checked it was in 'Inactive' state, so...
          Hide
          Philip Zeyliger added a comment -

          In other places, I've seen things marked as public with a comment (we could also do annotation) indicating that it ought to be used for testing only. If the package is server.datanode, that's pretty clearly internal to HDFS, no?

          I'm merely curious about the preferred coding style for this codebase. Definitely something JSR 294 ("superpackages") will help address.

          – Philip

          Show
          Philip Zeyliger added a comment - In other places, I've seen things marked as public with a comment (we could also do annotation) indicating that it ought to be used for testing only. If the package is server.datanode, that's pretty clearly internal to HDFS, no? I'm merely curious about the preferred coding style for this codebase. Definitely something JSR 294 ("superpackages") will help address. – Philip
          Konstantin Boudnik made changes -
          Attachment HDFS-705.patch [ 12422147 ]
          Hide
          Konstantin Boudnik added a comment -

          This is the patch for the issue

          Show
          Konstantin Boudnik added a comment - This is the patch for the issue
          Hide
          Konstantin Boudnik added a comment -

          It doesn't actually 'relates' but rather is very similar in its intentions

          Show
          Konstantin Boudnik added a comment - It doesn't actually 'relates' but rather is very similar in its intentions
          Konstantin Boudnik made changes -
          Field Original Value New Value
          Link This issue relates to HDFS-563 [ HDFS-563 ]
          Konstantin Boudnik created issue -

            People

            • Assignee:
              Konstantin Boudnik
              Reporter:
              Konstantin Boudnik
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development