Details

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

      Description

      This is introduced by HADOOP-5528.
      All of them about using unparametrized types.

      1. 5604-0.patch
        2 kB
        Chris Douglas

        Issue Links

          Activity

          Hide
          Konstantin Shvachko added a comment -

          No, I don't think we should suppress the warnings.
          The test should be written correctly using parametrized types.

          Show
          Konstantin Shvachko added a comment - No, I don't think we should suppress the warnings. The test should be written correctly using parametrized types.
          Hide
          Chris Douglas added a comment -

          The value type is ignored by the partitioner and wholly irrelevant. Supplying a type for the null values passed in for the unit test is pointless.

          Show
          Chris Douglas added a comment - The value type is ignored by the partitioner and wholly irrelevant. Supplying a type for the null values passed in for the unit test is pointless.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12404279/5604-0.patch
          against trunk revision 760783.

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

          +1 tests included. The patch appears to include 6 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 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/88/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/88/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/88/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/88/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/12404279/5604-0.patch against trunk revision 760783. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/88/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/88/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/88/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/88/console This message is automatically generated.
          Hide
          Konstantin Shvachko added a comment -

          If a method is always called with a parameter == null, then the class needs another method that does not have this parameter. Especially if this method does not use the parameter at all.

          Show
          Konstantin Shvachko added a comment - If a method is always called with a parameter == null, then the class needs another method that does not have this parameter. Especially if this method does not use the parameter at all.
          Hide
          Konstantin Shvachko added a comment -

          HADOOP-5528 should have been reversed.
          In addition to 32 javac warnings, this patch introduces a new AND already deprecated class. It also introduces two identical tests located in two different packages, which are produced by code replication.
          It needs to be re-reviewed.

          Show
          Konstantin Shvachko added a comment - HADOOP-5528 should have been reversed. In addition to 32 javac warnings, this patch introduces a new AND already deprecated class. It also introduces two identical tests located in two different packages, which are produced by code replication. It needs to be re-reviewed.
          Hide
          Klaas Bosteels added a comment -

          Konstantin:

          • The deprecated class is there because we want to support both the new and the old mapreduce API. This does make sense, in my opinion, since the old API is still used heavily (personally I'm using the BinaryPartitioner from Streaming, for instance, which still uses the old API and might continue doing so for while). The deprecated version just extends the one using the new API though, so there's not code duplication.
          • The null parameter is just because the value parameter is irrelevant for this particular partitioner, but obviously we do want it to take this parameter because it has to implement the partitioner interface.
          • The tests are indeed replicated, but they are not identical, since one of them uses the deprecated class while the other uses the class from the new API. In theory it would be sufficient to test only the non-deprecated class (because the deprecated one extends the non-deprecated one), but it felt safer to have a unit test for both.

          I was aware that the unit tests added by the patch generate javac warnings, but I thought javac warnings for unit tests were not considered harmful. We could suppress the warnings like Chris did in his patch, but apart from that I don't think anything should change.

          Show
          Klaas Bosteels added a comment - Konstantin: The deprecated class is there because we want to support both the new and the old mapreduce API. This does make sense, in my opinion, since the old API is still used heavily (personally I'm using the BinaryPartitioner from Streaming, for instance, which still uses the old API and might continue doing so for while). The deprecated version just extends the one using the new API though, so there's not code duplication. The null parameter is just because the value parameter is irrelevant for this particular partitioner, but obviously we do want it to take this parameter because it has to implement the partitioner interface. The tests are indeed replicated, but they are not identical, since one of them uses the deprecated class while the other uses the class from the new API. In theory it would be sufficient to test only the non-deprecated class (because the deprecated one extends the non-deprecated one), but it felt safer to have a unit test for both. I was aware that the unit tests added by the patch generate javac warnings, but I thought javac warnings for unit tests were not considered harmful. We could suppress the warnings like Chris did in his patch, but apart from that I don't think anything should change.
          Hide
          Klaas Bosteels added a comment -

          Fixed by the latest patch for HADOOP-5528.

          Show
          Klaas Bosteels added a comment - Fixed by the latest patch for HADOOP-5528 .

            People

            • Assignee:
              Unassigned
              Reporter:
              Konstantin Shvachko
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development