Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.20.2
    • Component/s: build
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Removes javac warnings by either resolving them or suppressing them (wherever resolution is not possible)

      Description

      Towards a solution for HADOOP-5628, we need to resolve all javac warnings. This jira will try to resolve javac warnings where ever possible and suppress them where resolution is not possible.

      1. mapreduce-623.patch
        27 kB
        Jothi Padmanabhan
      2. M623-0v20.patch
        22 kB
        Chris Douglas
      3. M623-0y20.patch
        21 kB
        Chris Douglas

        Issue Links

          Activity

          Jothi Padmanabhan created issue -
          Jothi Padmanabhan made changes -
          Field Original Value New Value
          Link This issue blocks HADOOP-5628 [ HADOOP-5628 ]
          Hide
          Jothi Padmanabhan added a comment -

          While working on this, we stumbled on a javac bug where deprecated warnings do not get suppressed when deprecated classes are used as method arguments. This apparently is a known bug. The following code illustrates the problem – warning in f1 is suppressed, but not in f2

          $ cat A.java
          @SuppressWarnings("deprecation")
          public class A {
            public  void f1 () {
              B b = new B("abc");
              b.fun();
            }
          
            public void f2(B b) {
               b.fun();
            }
          
          }
          
          $ cat B.java
          @Deprecated
          public class B {
          private String s;
          public B(String str) { s = str;}
          public void fun() { System.out.println(s);}
          
          }
          
          
          $ javac -g -verbose -Xlint A.java B.java
          .
          .
          A.java:8: warning: [deprecation] B in unnamed package has been
          deprecated
            public void f2(B b) {
                           ^ 
          
          
          Show
          Jothi Padmanabhan added a comment - While working on this, we stumbled on a javac bug where deprecated warnings do not get suppressed when deprecated classes are used as method arguments. This apparently is a known bug . The following code illustrates the problem – warning in f1 is suppressed, but not in f2 $ cat A.java @SuppressWarnings( "deprecation" ) public class A { public void f1 () { B b = new B( "abc" ); b.fun(); } public void f2(B b) { b.fun(); } } $ cat B.java @Deprecated public class B { private String s; public B( String str) { s = str;} public void fun() { System .out.println(s);} } $ javac -g -verbose -Xlint A.java B.java . . A.java:8: warning: [deprecation] B in unnamed package has been deprecated public void f2(B b) { ^
          Hide
          Jothi Padmanabhan added a comment -

          To make progress on this front, in spite of the javac bug, we could possibly:

          1. Wait till all the framework has been changed to use new api, so the number of deprecated warnings automatically reaches zero
          2. Modify the test-patch script to ignore deprecated warnings (something like grep "warning:' | grep -v deprecated | wc -l)

          Thoughts?

          Show
          Jothi Padmanabhan added a comment - To make progress on this front, in spite of the javac bug, we could possibly: Wait till all the framework has been changed to use new api, so the number of deprecated warnings automatically reaches zero Modify the test-patch script to ignore deprecated warnings (something like grep "warning:' | grep -v deprecated | wc -l) Thoughts?
          Jothi Padmanabhan made changes -
          Component/s mapred [ 12310690 ]
          Owen O'Malley made changes -
          Project Hadoop Common [ 12310240 ] Hadoop Map/Reduce [ 12310941 ]
          Key HADOOP-5663 MAPREDUCE-623
          Component/s mapred [ 12310690 ]
          Jothi Padmanabhan made changes -
          Link This issue blocks MAPREDUCE-670 [ MAPREDUCE-670 ]
          Hide
          Jothi Padmanabhan added a comment -

          Patch that removes all but deprecated javac warnings.

          Since we have a slew of deprecated warnings, should we just modify test-patch.sh to do a blanket suppression of all deprecated warnings as suggested here

          Show
          Jothi Padmanabhan added a comment - Patch that removes all but deprecated javac warnings. Since we have a slew of deprecated warnings, should we just modify test-patch.sh to do a blanket suppression of all deprecated warnings as suggested here
          Jothi Padmanabhan made changes -
          Attachment mapreduce-623.patch [ 12412269 ]
          Jothi Padmanabhan made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > 2. Modify the test-patch script to ignore deprecated warnings (something like grep "warning:' | grep -v deprecated | wc -l)

          -1 for this since deprecated warnings in general should be fixed but not ignored.

          Show
          Tsz Wo Nicholas Sze added a comment - > 2. Modify the test-patch script to ignore deprecated warnings (something like grep "warning:' | grep -v deprecated | wc -l) -1 for this since deprecated warnings in general should be fixed but not ignored.
          Hide
          Jakob Homan added a comment -

          I also ran into a variation of this in HADOOP-5877, where you can't suppress warnings on fields. Those fixes were committed with resolving what to do about about not being able to remove the warning.

          Show
          Jakob Homan added a comment - I also ran into a variation of this in HADOOP-5877 , where you can't suppress warnings on fields. Those fixes were committed with resolving what to do about about not being able to remove the warning.
          Hide
          Jothi Padmanabhan added a comment -

          -1 for this since deprecated warnings in general should be fixed but not ignored.

          Sure, that is definitely the ideal way, but given that we are generating close to 1000 deprecated warnings because of the move from Old to New API, it might actually take a while before we could fix all of those. Should we blanket disable them now and then reevaluate once the framework code has been updated to use only New API objects?

          Show
          Jothi Padmanabhan added a comment - -1 for this since deprecated warnings in general should be fixed but not ignored. Sure, that is definitely the ideal way, but given that we are generating close to 1000 deprecated warnings because of the move from Old to New API, it might actually take a while before we could fix all of those. Should we blanket disable them now and then reevaluate once the framework code has been updated to use only New API objects?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > ... Should we blanket disable them now and then reevaluate once the framework code has been updated to use only New API objects?

          test-patch.sh is shared by common, hdfs and mapreduce. So I am -1 on changing it for a mapreduce specific problem.

          I agree that it is hard to fix 1000 deprecated warnings at once. It may be good to disable the checking in mapreduce as a temporary fix. I suggest that the deprecated warnings should be shown somewhere even they are disabled. Otherwise, it will be hard to keep track of them.

          Show
          Tsz Wo Nicholas Sze added a comment - > ... Should we blanket disable them now and then reevaluate once the framework code has been updated to use only New API objects? test-patch.sh is shared by common, hdfs and mapreduce. So I am -1 on changing it for a mapreduce specific problem. I agree that it is hard to fix 1000 deprecated warnings at once. It may be good to disable the checking in mapreduce as a temporary fix. I suggest that the deprecated warnings should be shown somewhere even they are disabled. Otherwise, it will be hard to keep track of them.
          Owen O'Malley made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Owen O'Malley added a comment -

          resubmit to hudson.

          Show
          Owen O'Malley added a comment - resubmit to hudson.
          Owen O'Malley made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Owen O'Malley added a comment -

          let's get the code changes committed and then debate the approach on the deprecated warnings.

          Show
          Owen O'Malley added a comment - let's get the code changes committed and then debate the approach on the deprecated warnings.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > let's get the code changes committed and then debate the approach on the deprecated warnings.

          yes, we could deal with the deprecated warnings in a separated issue.

          Show
          Tsz Wo Nicholas Sze added a comment - > let's get the code changes committed and then debate the approach on the deprecated warnings. yes, we could deal with the deprecated warnings in a separated issue.
          Hide
          Jothi Padmanabhan added a comment -

          We could possibly do the "blanket suppression" only for mapred after we Giri get this https://issues.apache.org/jira/browse/HADOOP-6137 checked in.

          Show
          Jothi Padmanabhan added a comment - We could possibly do the "blanket suppression" only for mapred after we Giri get this https://issues.apache.org/jira/browse/HADOOP-6137 checked in.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12412269/mapreduce-623.patch
          against trunk revision 792490.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +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 passed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/367/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/367/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/367/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/367/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/12412269/mapreduce-623.patch against trunk revision 792490. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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 passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/367/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/367/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/367/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/367/console This message is automatically generated.
          Hide
          Jothi Padmanabhan added a comment -

          Contrib test failures are all streaming tests that have been failing for a while.
          This patch just removes javac warnings and does not add/modify any new features, hence no test is required.

          Show
          Jothi Padmanabhan added a comment - Contrib test failures are all streaming tests that have been failing for a while. This patch just removes javac warnings and does not add/modify any new features, hence no test is required.
          Sharad Agarwal made changes -
          Issue Type Bug [ 1 ] Improvement [ 4 ]
          Hide
          Sharad Agarwal added a comment -

          I committed this. Thanks Jothi!

          Show
          Sharad Agarwal added a comment - I committed this. Thanks Jothi!
          Sharad Agarwal made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Fix Version/s 0.21.0 [ 12314045 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

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

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #20 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/20/ )
          Jothi Padmanabhan made changes -
          Release Note Removes javac warnings by either resolving them or suppressing them (wherever resolution is not possible)
          Component/s build [ 12312909 ]
          Daniel Dai made changes -
          Link This issue relates to PIG-1033 [ PIG-1033 ]
          Hide
          Chris Douglas added a comment -

          Patch for 0.20. Differences from original:

          • Did not deprecate JobHistory::init
          • Did not deprecate jobcontrol.Job::setAssignedJobID which was deprecated in MAPREDUCE-368 in 0.21
          • Several classes in mapreduce.lib ported in 0.21 are not in 0.20 (TotalOrderPartitioner, aggregate.ValueHistogram, etc.)
          Show
          Chris Douglas added a comment - Patch for 0.20. Differences from original: Did not deprecate JobHistory::init Did not deprecate jobcontrol.Job::setAssignedJobID which was deprecated in MAPREDUCE-368 in 0.21 Several classes in mapreduce.lib ported in 0.21 are not in 0.20 (TotalOrderPartitioner, aggregate.ValueHistogram, etc.)
          Chris Douglas made changes -
          Attachment M623-0v20.patch [ 12435674 ]
          Hide
          Chris Douglas added a comment -

          These changes should also be applied to 0.20

          Show
          Chris Douglas added a comment - These changes should also be applied to 0.20
          Chris Douglas made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Hide
          Todd Lipcon added a comment -

          Patch looks good to me. Only tiny nit I have is that the indentation got screwed up by once space on JobHistory.logSubmitted. If you just fix that before commit, no need to post a new patch in my opinion.

          Show
          Todd Lipcon added a comment - Patch looks good to me. Only tiny nit I have is that the indentation got screwed up by once space on JobHistory.logSubmitted. If you just fix that before commit, no need to post a new patch in my opinion.
          Hide
          Owen O'Malley added a comment -

          +1 on the patch, although I tend to prefer @SuppressWarnings("serial") instead of adding the serialVersionUID.

          Show
          Owen O'Malley added a comment - +1 on the patch, although I tend to prefer @SuppressWarnings("serial") instead of adding the serialVersionUID.
          Hide
          Chris Douglas added a comment -

          I committed this to the 0.20 branch

          Show
          Chris Douglas added a comment - I committed this to the 0.20 branch
          Chris Douglas made changes -
          Resolution Fixed [ 1 ]
          Status Reopened [ 4 ] Resolved [ 5 ]
          Fix Version/s 0.20.2 [ 12314205 ]
          Fix Version/s 0.21.0 [ 12314045 ]
          Chris Douglas made changes -
          Attachment M623-0y20.patch [ 12439215 ]

            People

            • Assignee:
              Jothi Padmanabhan
              Reporter:
              Jothi Padmanabhan
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development