Details

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

      Description

      These files have a number of javac 'class depricated' warnings
      src/test/hdfs-with-mr/org/apache/hadoop/hdfs/NNBench.java
      src/test/hdfs-with-mr/org/apache/hadoop/hdfs/NNBenchWithoutMR.java
      It is possible to fix most of them plus make some readability improvements on the code.

      1. HADOOP-5867.patch
        16 kB
        Konstantin Boudnik
      2. HADOOP-5867.patch
        13 kB
        Konstantin Boudnik
      3. HADOOP-5867.patch
        13 kB
        Konstantin Boudnik

        Issue Links

          Activity

          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this. Thanks, Konstantin!

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this. Thanks, Konstantin!
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12408546/HADOOP-5867.patch
          against trunk revision 777152.

          +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-vesta.apache.org/372/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/372/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/372/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/372/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/12408546/HADOOP-5867.patch against trunk revision 777152. +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-vesta.apache.org/372/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/372/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/372/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/372/console This message is automatically generated.
          Hide
          Konstantin Boudnik added a comment - - edited

          These changes aren't exactly touch any kind of tests, however I have SUCCESSFULLY ran the following tests

          • test-hdfs-with-mr
          • test-hdfs

          Also I have ran NNBench and NNBenchWithoutMR successfully.

          Please let me know if something beyond this has to be done before the commit of the patch.

          Show
          Konstantin Boudnik added a comment - - edited These changes aren't exactly touch any kind of tests, however I have SUCCESSFULLY ran the following tests test-hdfs-with-mr test-hdfs Also I have ran NNBench and NNBenchWithoutMR successfully. Please let me know if something beyond this has to be done before the commit of the patch.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > More generic question is: are we trying to count both false and true return as success and only exception throwing as a failure?
          I am fine with counting both false and true as long as the options are carefully considered.

          +1 patch looks good

          Show
          Tsz Wo Nicholas Sze added a comment - > More generic question is: are we trying to count both false and true return as success and only exception throwing as a failure? I am fine with counting both false and true as long as the options are carefully considered. +1 patch looks good
          Hide
          Konstantin Boudnik added a comment -

          All but one comments from the reviewer were taken into consideration and fixed.
          I'm hesitant to used suggestion from the review and modify the code in question to
          success = fileSys.rename(...)
          because rename() might return false in three cases our of four existing exit points.

          More generic question is: are we trying to count both false and true return as success and only exception throwing as a failure?

          Show
          Konstantin Boudnik added a comment - All but one comments from the reviewer were taken into consideration and fixed. I'm hesitant to used suggestion from the review and modify the code in question to success = fileSys.rename(...) because rename() might return false in three cases our of four existing exit points. More generic question is: are we trying to count both false and true return as success and only exception throwing as a failure?
          Hide
          Konstantin Boudnik added a comment -

          Thanks for review Nicholas.

          Some comments:
          1) Oops, will fix long lines
          2) I though of making exactly this change (the first version of the patch
          contains this very change). However, it might be troublesome in some cases,
          because fileSys.rename can return false in some cases (say, namespace won't
          rename the path) and then we might stuck in this loop forever
          (code)
          do {
          ...
          while (!success);
          (/code)
          That's why I've left it as is and put this comment in both (rename and delete)
          cases. I'll appreciate to hear from you on this before I'll submit the update to
          this patch.

          3) nice catch! Fixed along with a couple of other excessive castings to double.

          Thanks in advance,
          Cos

          Show
          Konstantin Boudnik added a comment - Thanks for review Nicholas. Some comments: 1) Oops, will fix long lines 2) I though of making exactly this change (the first version of the patch contains this very change). However, it might be troublesome in some cases, because fileSys.rename can return false in some cases (say, namespace won't rename the path) and then we might stuck in this loop forever (code) do { ... while (!success); (/code) That's why I've left it as is and put this comment in both (rename and delete) cases. I'll appreciate to hear from you on this before I'll submit the update to this patch. 3) nice catch! Fixed along with a couple of other excessive castings to double. Thanks in advance, Cos
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Patch looks mostly good. Some nits:

          • Each line should have 80 characters or less. Some lines are too long, e.g.
            +     * @return true if the method was able to sleep for <code>-startTime</code> without interruption; false otherwise
            
          • Question: the rename and delete return values are ignored currently. How about setting "success = fileSys.rename(..)"?
          • Possible bug in the existing code: "totalTimeTPmS / successfulFileOps" below is an integer division
                double AverageExecutionTime = (totalTimeTPmS == 0) ?
                    (double) successfulFileOps : 
                    (double) (totalTimeTPmS / successfulFileOps);
            
          Show
          Tsz Wo Nicholas Sze added a comment - Patch looks mostly good. Some nits: Each line should have 80 characters or less. Some lines are too long, e.g. + * @return true if the method was able to sleep for <code>-startTime</code> without interruption; false otherwise Question: the rename and delete return values are ignored currently. How about setting "success = fileSys.rename(..)"? Possible bug in the existing code: "totalTimeTPmS / successfulFileOps" below is an integer division double AverageExecutionTime = (totalTimeTPmS == 0) ? ( double ) successfulFileOps : ( double ) (totalTimeTPmS / successfulFileOps);
          Hide
          Konstantin Boudnik added a comment -

          The following modifications were made:

          • redundant initializations were removed
          • missed JavaDoc added
          • unused variables were removed and replaced with proper comments explaining why the result of some calls isn't important for the purpose of the test
          • some class members were converted to a method's variables
          • few method signatures were refactored to remove unused parameters
          • redundant casts were removed
          Show
          Konstantin Boudnik added a comment - The following modifications were made: redundant initializations were removed missed JavaDoc added unused variables were removed and replaced with proper comments explaining why the result of some calls isn't important for the purpose of the test some class members were converted to a method's variables few method signatures were refactored to remove unused parameters redundant casts were removed
          Hide
          Konstantin Boudnik added a comment -

          Fixing pretty much all javac warnings unrelated to deprecation of mapred APIs. Those will have to be addressed separately

          Show
          Konstantin Boudnik added a comment - Fixing pretty much all javac warnings unrelated to deprecation of mapred APIs. Those will have to be addressed separately
          Hide
          Konstantin Boudnik added a comment -

          A number of warnings in NNBench.java is caused by deprecation of JobConf and its replacement by Configuration. However, these two aren't fully compatible and it seems that this improvement should be addressed by HADOOP-5343 along with other similar issues

          Show
          Konstantin Boudnik added a comment - A number of warnings in NNBench.java is caused by deprecation of JobConf and its replacement by Configuration. However, these two aren't fully compatible and it seems that this improvement should be addressed by HADOOP-5343 along with other similar issues

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development