Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-2193

13 Findbugs warnings on trunk and branch-0.22

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Not a Problem
    • Affects Version/s: 0.22.0, 0.23.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Tags:
      findbugs

      Description

      There are 13 findbugs warnings on trunk. See attached html file. These must be fixed or filtered out to get back to 0 warnings. The OK_FINDBUGS_WARNINGS property in src/test/test-patch.properties should also be set to 0 in the patch that fixes this issue.

      1. findbugsWarnings.html
        24 kB
        Nigel Daley
      2. hadoop-findbugs-report.html
        9 kB
        Harsh J
      3. hadoop-findbugs-report.html
        3 kB
        Harsh J
      4. hadoop-findbugs-report.html
        13 kB
        Harsh J
      5. mapreduce.trunk.findbugs.r1.diff
        11 kB
        Harsh J
      6. mapreduce.trunk.findbugs.r2.diff
        11 kB
        Harsh J
      7. mapreduce.trunk.findbugs.r3.diff
        7 kB
        Harsh J
      8. mapreduce.trunk.findbugs.r4.diff
        8 kB
        Harsh J
      9. mapreduce.trunk.findbugs.r5.diff
        9 kB
        Harsh J
      10. mapreduce.trunk.findbugs.r5.diff
        8 kB
        Harsh J
      11. mapreduce.trunk.findbugs.r6.diff
        10 kB
        Harsh J
      12. mapreduce.trunk.findbugs.r7.diff
        10 kB
        Harsh J

        Activity

        Hide
        Harsh J added a comment -

        Attempting this cause it looks like an interesting task for a newcomer.

        Attached a patch that filters and fixes some of these warnings. [Also attached the resultant warning HTML page].
        Please review

        Regarding the four remaining IS2_INCONSISTENT_SYNC warnings, I'm unable to think of a proper way to exclude them (as I think getTasks() doesn't require to be synchronized).
        I tried performing a Match on the Method, but findbugs is only interested in the Field as a whole. Ignoring the Fields (maps, reduces, setup, cleanup) for the entire JIP class doesn't look like a good idea to me. Any tips on how to resolve this filtering?

        Show
        Harsh J added a comment - Attempting this cause it looks like an interesting task for a newcomer. Attached a patch that filters and fixes some of these warnings. [Also attached the resultant warning HTML page] . Please review Regarding the four remaining IS2_INCONSISTENT_SYNC warnings, I'm unable to think of a proper way to exclude them (as I think getTasks() doesn't require to be synchronized). I tried performing a Match on the Method, but findbugs is only interested in the Field as a whole. Ignoring the Fields (maps, reduces, setup, cleanup) for the entire JIP class doesn't look like a good idea to me. Any tips on how to resolve this filtering?
        Hide
        Harsh J added a comment -

        I had a look at JIP's use of those TIP arrays again, and I guess that it is alright to ignore non sync access to them via getTasks() as getTasks() is thread safe. But I may be wrong, so please review. Marking as PA.

        Show
        Harsh J added a comment - I had a look at JIP's use of those TIP arrays again, and I guess that it is alright to ignore non sync access to them via getTasks() as getTasks() is thread safe. But I may be wrong, so please review. Marking as PA.
        Hide
        Todd Lipcon added a comment -

        In Localizer, I don't think it's correct to remove the synchronization on localizedUser. findbugs thinks it's suspicious that it's synchronizing on an AtomicBoolean, but we're not even using AtomicBoolean for its atomicity, we just need some kind of "holder object" for the boolean and AtomicBoolean is a handy one. Maybe instead we could use a single-element array? Or define Holder<T> somewhere in MR (this isn't the only place it would be handy)

        Regarding JobInProgress, the synchronization here is pretty messed up... mostly because JIP is often externally synchronized upon, or done under the JT-wide or scheduler-wide lock. I think we should just set getTasks to be synchronized rather than ignoring this.

        Show
        Todd Lipcon added a comment - In Localizer, I don't think it's correct to remove the synchronization on localizedUser. findbugs thinks it's suspicious that it's synchronizing on an AtomicBoolean, but we're not even using AtomicBoolean for its atomicity, we just need some kind of "holder object" for the boolean and AtomicBoolean is a handy one. Maybe instead we could use a single-element array? Or define Holder<T> somewhere in MR (this isn't the only place it would be handy) Regarding JobInProgress, the synchronization here is pretty messed up... mostly because JIP is often externally synchronized upon, or done under the JT-wide or scheduler-wide lock. I think we should just set getTasks to be synchronized rather than ignoring this.
        Hide
        Harsh J added a comment -

        Thanks for the review Todd. I've made the getTasks synchronized now and also have removed the ignores.

        About the Holder, must I write the class myself? And if it is going to be used in other places as well, where must I put it (package/project wise)?

        Show
        Harsh J added a comment - Thanks for the review Todd. I've made the getTasks synchronized now and also have removed the ignores. About the Holder, must I write the class myself? And if it is going to be used in other places as well, where must I put it (package/project wise)?
        Hide
        Harsh J added a comment -

        New patch that tries to resolve Todd's additional comments.

        • Adds a new class 'Holder<T>' added in o.a.h.mapreduce package, for use right now only in Localizer.java. Re-added synchronized block in Localizer.java.
        • Removed ignores on maps/reduces/etc TIP arrays in JobInProgress.java and made the getTasks() method synchronized.

        Checkstyle passes on Holder.java with 0 errors/warnings (via the report of `ant checkstyle`). Findbugs reports 0 errors, like the previous attachment.

        Show
        Harsh J added a comment - New patch that tries to resolve Todd's additional comments. Adds a new class 'Holder<T>' added in o.a.h.mapreduce package, for use right now only in Localizer.java. Re-added synchronized block in Localizer.java. Removed ignores on maps/reduces/etc TIP arrays in JobInProgress.java and made the getTasks() method synchronized. Checkstyle passes on Holder.java with 0 errors/warnings (via the report of `ant checkstyle`). Findbugs reports 0 errors, like the previous attachment.
        Hide
        Todd Lipcon added a comment -

        Hey Harsh, thanks for making the changes. A few more notes:

        • Holder needs the Apache license and an interface annotation - probably Stable and Private. It should also move into the o.a.h.mapreduce.util package - o.a.h.mapreduce is for public APIs as far as I understand
        • I'd like someone else to take a look at adding 'synchronized' to getTasks() in addition to me - it's just scary enough that I think an extra reviewer is prudent.
        Show
        Todd Lipcon added a comment - Hey Harsh, thanks for making the changes. A few more notes: Holder needs the Apache license and an interface annotation - probably Stable and Private. It should also move into the o.a.h.mapreduce.util package - o.a.h.mapreduce is for public APIs as far as I understand I'd like someone else to take a look at adding 'synchronized' to getTasks() in addition to me - it's just scary enough that I think an extra reviewer is prudent.
        Hide
        Harsh J added a comment -

        Another new patch (Rev 4.)

        • Moved Holder from o.a.h.mapreduce to o.a.h.mapreduce.util
        • Added the forgotten-about ASF license and API interface annotations, as per Todd's comments

        ant clean findbugs -Dfindbugs.home=/opt/findbugs -> 0, as before.

        Waiting for another review for the 'synchronized' change made in the TIP[] JobInProgress::getTasks(TaskType) method.

        Show
        Harsh J added a comment - Another new patch (Rev 4.) Moved Holder from o.a.h.mapreduce to o.a.h.mapreduce.util Added the forgotten-about ASF license and API interface annotations, as per Todd's comments ant clean findbugs -Dfindbugs.home=/opt/findbugs -> 0, as before. Waiting for another review for the 'synchronized' change made in the TIP[] JobInProgress::getTasks(TaskType) method.
        Hide
        Priyo Mustafi added a comment -

        Looks like https://issues.apache.org/jira/browse/MAPREDUCE-2026 removed synchronization from JobInProgress.getCounters() to make the lock more granular. This has added two findbugs now. Should these be ignored then as it was done on purpose?

        Show
        Priyo Mustafi added a comment - Looks like https://issues.apache.org/jira/browse/MAPREDUCE-2026 removed synchronization from JobInProgress.getCounters() to make the lock more granular. This has added two findbugs now. Should these be ignored then as it was done on purpose?
        Hide
        Harsh J added a comment -

        It may be ignored if it is acceptable to do so, but via the findbugs filter XML, there's no way of doing selective-ignore without ignoring the whole field itself (no line number sub-filter either). I don't think ignoring the fields forever would be a good idea for changes that are yet to come in that class.

        Show
        Harsh J added a comment - It may be ignored if it is acceptable to do so, but via the findbugs filter XML, there's no way of doing selective-ignore without ignoring the whole field itself (no line number sub-filter either). I don't think ignoring the fields forever would be a good idea for changes that are yet to come in that class.
        Hide
        Chris Douglas added a comment -

        I'd like someone else to take a look at adding 'synchronized' to getTasks() in addition to me - it's just scary enough that I think an extra reviewer is prudent.

        I tried to find some history: http://s.apache.org/T99 (from MAPREDUCE-1316). The synchronization was intentionally left out, but only to be consistent with the existing code.

        The synchronization around JIP is difficult to track. Its omission here appears to admit the possibility that a query of an initializing job could iterate over a partially constructed set of TIPs and cause NPEs (e.g. via the JSP), but I couldn't find a place where this is a serious problem. After the job is initialized, none of the references to member fields become stale. On the other hand, I looked through all the callers of getTasks(), and most are holding the JT lock already (mostly schedulers, during heartbeat processing) so I doubt that synchronizing on the JIP here would cause problems.

        One could fix getTasks() to be threadsafe by changing create

        {Map,Reduce}

        Tasks and initSetupCleanupTasks to assign the initialized array of tasks to the member field. The current behavior constructs it in-place, which (AFAICT) means that a caller of getTasks() could read partially-constructed TIPs. It's probably better to make getTasks() synchronized, to avoid having getTasks() return partially constructed, inconsistent JIP state.

        Show
        Chris Douglas added a comment - I'd like someone else to take a look at adding 'synchronized' to getTasks() in addition to me - it's just scary enough that I think an extra reviewer is prudent. I tried to find some history: http://s.apache.org/T99 (from MAPREDUCE-1316 ). The synchronization was intentionally left out, but only to be consistent with the existing code. The synchronization around JIP is difficult to track. Its omission here appears to admit the possibility that a query of an initializing job could iterate over a partially constructed set of TIPs and cause NPEs (e.g. via the JSP), but I couldn't find a place where this is a serious problem. After the job is initialized, none of the references to member fields become stale. On the other hand, I looked through all the callers of getTasks(), and most are holding the JT lock already (mostly schedulers, during heartbeat processing) so I doubt that synchronizing on the JIP here would cause problems. One could fix getTasks() to be threadsafe by changing create {Map,Reduce} Tasks and initSetupCleanupTasks to assign the initialized array of tasks to the member field. The current behavior constructs it in-place, which (AFAICT) means that a caller of getTasks() could read partially-constructed TIPs. It's probably better to make getTasks() synchronized, to avoid having getTasks() return partially constructed, inconsistent JIP state.
        Hide
        Harsh J added a comment -

        New patch (still leaves a couple of sync warnings left, as introduced by MAPREDUCE-2026).

        @Chris - I've made the changes you suggested. Although, even with those changes, is it required for getTasks(TaskType) to be sync'd? I've sync'd it in this patch, but let me know if it can be removed, and the warning ignored (possibly inaccurate, as admitted by Findbugs).

        @Todd - Shall I open a new JIRA for other places Holder<T> may be used at?

        The additional two findbug warnings generated by the issue pointed by Priyo (MAPREDUCE-2026 - Interesting why it does not show up in the test-patch?) needs to be ignored/re-reviewed as well (for getMapCounters/getReduceCounters are left synchronized - why?).

        Show
        Harsh J added a comment - New patch (still leaves a couple of sync warnings left, as introduced by MAPREDUCE-2026 ). @Chris - I've made the changes you suggested. Although, even with those changes, is it required for getTasks(TaskType) to be sync'd? I've sync'd it in this patch, but let me know if it can be removed, and the warning ignored (possibly inaccurate, as admitted by Findbugs). @Todd - Shall I open a new JIRA for other places Holder<T> may be used at? The additional two findbug warnings generated by the issue pointed by Priyo ( MAPREDUCE-2026 - Interesting why it does not show up in the test-patch?) needs to be ignored/re-reviewed as well (for getMapCounters / getReduceCounters are left synchronized - why?).
        Hide
        Harsh J added a comment -

        Fresh Findbugs report for previous patch.

        Show
        Harsh J added a comment - Fresh Findbugs report for previous patch.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12471957/mapreduce.trunk.findbugs.r5.diff
        against trunk revision 1074251.

        +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 patch appears to cause tar ant target to fail.

        -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

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

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

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

        -1 system test framework. The patch failed system test framework compile.

        Test results: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/53//testReport/
        Console output: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/53//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/12471957/mapreduce.trunk.findbugs.r5.diff against trunk revision 1074251. +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 patch appears to cause tar ant target to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: -1 contrib tests. The patch failed contrib unit tests. -1 system test framework. The patch failed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/53//testReport/ Console output: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/53//console This message is automatically generated.
        Hide
        Harsh J added a comment -

        Oops, looks like my diff failed to contain the Holder file. Forgot to add it to svn before diff.

        Uploading a proper patch and setting to 'In Progress'.

        Show
        Harsh J added a comment - Oops, looks like my diff failed to contain the Holder file. Forgot to add it to svn before diff. Uploading a proper patch and setting to 'In Progress'.
        Hide
        Nigel Daley added a comment -

        Not a blocker for 0.22. Removing from 0.22.

        Show
        Nigel Daley added a comment - Not a blocker for 0.22. Removing from 0.22.
        Hide
        Harsh J added a comment -

        Rebased patch with OK_FINDBUGS_WARNINGS bumped to 2 to cover the JobInProgress's IS2 warnings that are non-ignorable.

        (Same report as last one)

        Show
        Harsh J added a comment - Rebased patch with OK_FINDBUGS_WARNINGS bumped to 2 to cover the JobInProgress's IS2 warnings that are non-ignorable. (Same report as last one)
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12483812/mapreduce.trunk.findbugs.r7.diff
        against trunk revision 1139400.

        +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 appears to introduce -11 new Findbugs (version 1.3.9) warnings.

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

        -1 core tests. The patch failed these core unit tests:
        org.apache.hadoop.cli.TestMRCLI
        org.apache.hadoop.fs.TestFileSystem

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

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/417//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/417//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/417//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/12483812/mapreduce.trunk.findbugs.r7.diff against trunk revision 1139400. +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 appears to introduce -11 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.cli.TestMRCLI org.apache.hadoop.fs.TestFileSystem -1 contrib tests. The patch failed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/417//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/417//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/417//console This message is automatically generated.
        Hide
        Harsh J added a comment -

        Failing core tests look unrelated to this patch (FsShell re-org work perhaps?)

        Show
        Harsh J added a comment - Failing core tests look unrelated to this patch (FsShell re-org work perhaps?)
        Hide
        Arun C Murthy added a comment -

        Sorry to come in late, the patch has gone stale. Can you please rebase? Thanks.

        Show
        Arun C Murthy added a comment - Sorry to come in late, the patch has gone stale. Can you please rebase? Thanks.
        Hide
        Harsh J added a comment -

        Looks to be in contention with MAPREDUCE-2908 efforts?

        Show
        Harsh J added a comment - Looks to be in contention with MAPREDUCE-2908 efforts?
        Hide
        Vinod Kumar Vavilapalli added a comment -

        Looks to be in contention with MAPREDUCE-2908 efforts?

        MAPREDUCE-2908 fixes the warnings in MRV2. Looks like this one is for those in MRV1.

        Show
        Vinod Kumar Vavilapalli added a comment - Looks to be in contention with MAPREDUCE-2908 efforts? MAPREDUCE-2908 fixes the warnings in MRV2. Looks like this one is for those in MRV1.
        Hide
        Harsh J added a comment -

        Ah yup. I suppose its worth going in anyway. Will rebase this for 0.22 alone for now, and forward-port.

        Show
        Harsh J added a comment - Ah yup. I suppose its worth going in anyway. Will rebase this for 0.22 alone for now, and forward-port.
        Hide
        Harsh J added a comment -

        I'm not sure if this is still relevant. Unassigning as I may cause a blockage otherwise.

        Show
        Harsh J added a comment - I'm not sure if this is still relevant. Unassigning as I may cause a blockage otherwise.
        Hide
        Harsh J added a comment -

        This issue has gone stale. Trunk and 0.22 have diverged so much that its best to have newer JIRAs tracking it - if problems still exist (I know trunk has no findbugs issue presently). Resolving as 'Not a Problem'.

        Show
        Harsh J added a comment - This issue has gone stale. Trunk and 0.22 have diverged so much that its best to have newer JIRAs tracking it - if problems still exist (I know trunk has no findbugs issue presently). Resolving as 'Not a Problem'.

          People

          • Assignee:
            Unassigned
            Reporter:
            Nigel Daley
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development