HBase
  1. HBase
  2. HBASE-5598

Analyse and fix the findbugs reporting by QA and add invalid bugs into findbugs-excludeFilter file

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.92.1, 0.94.0, 0.95.2
    • Fix Version/s: 0.95.0
    • Component/s: scripts
    • Labels:
      None
    • Release Note:
      Hide
      Developers can generate the findbugs report for analysing the findbugs violations.
      Execute this maven command for running and generating the findbugs report: mvn findbugs:gui
      This generates the report in GUI, by which will directly show the code instances where exactly the issue is reported.

      If the developer finds any invalid bug, then that bug pattern can be updated in dev-support/findbugs-exclude.xml file.
      example:
      <Match>
         <Class name="org.apache.hadoop.hbase.regionserver.HRegion" />
         <Method name="getRecentFlushInfo" />
         <Bug pattern="UL_UNRELEASED_LOCK_EXCEPTION_PATH" />
      </Match>
      Show
      Developers can generate the findbugs report for analysing the findbugs violations. Execute this maven command for running and generating the findbugs report: mvn findbugs:gui This generates the report in GUI, by which will directly show the code instances where exactly the issue is reported. If the developer finds any invalid bug, then that bug pattern can be updated in dev-support/findbugs-exclude.xml file. example: <Match>    <Class name="org.apache.hadoop.hbase.regionserver.HRegion" />    <Method name="getRecentFlushInfo" />    <Bug pattern="UL_UNRELEASED_LOCK_EXCEPTION_PATH" /> </Match>
    • Tags:
      noob

      Description

      There are many findbugs errors reporting by HbaseQA. HBASE-5597 is going to up the OK count.
      This may lead to other issues when we re-factor the code, if we induce new valid ones and remove invalid bugs also can not be reported by QA.

      So, I would propose to add the exclude filter file for findbugs(for the invalid bugs). If we find any valid ones, we can fix under this JIRA.

      1. HBASE-5598.patch
        2 kB
        Uma Maheswara Rao G
      2. ASF.LICENSE.NOT.GRANTED--findbugs-gui-report.jpg
        175 kB
        Uma Maheswara Rao G
      3. 5598.part1.patch
        23 kB
        Nicolas Liochon
      4. 5598.part2.patch
        24 kB
        Nicolas Liochon
      5. 5598.part2.patch
        24 kB
        Nicolas Liochon

        Issue Links

          Activity

          Hide
          Jonathan Hsieh added a comment -

          Currently we are somewhere around the 770 warnings/errors mark. We should chop this into subtasks to break down the work and knock out related issues.

          For this to last, once this we get the findbugs warnings to 0, committers need to enforce a no-new-findbugs errors policy on reviews. Agreed?

          Show
          Jonathan Hsieh added a comment - Currently we are somewhere around the 770 warnings/errors mark. We should chop this into subtasks to break down the work and knock out related issues. For this to last, once this we get the findbugs warnings to 0, committers need to enforce a no-new-findbugs errors policy on reviews. Agreed?
          Hide
          stack added a comment -

          Propose up on dev list I'd say Jon. More folks will see your question.

          Show
          stack added a comment - Propose up on dev list I'd say Jon. More folks will see your question.
          Hide
          Lars Hofhansl added a comment -

          +1 on no-new-findbugs-policy once it's down to 0. (We do the same at Salesforce.)
          Not sure, though, that right method to get this to 0 is to use an exclude filter, should use findbug annotation in the files.

          Show
          Lars Hofhansl added a comment - +1 on no-new-findbugs-policy once it's down to 0. (We do the same at Salesforce.) Not sure, though, that right method to get this to 0 is to use an exclude filter, should use findbug annotation in the files.
          Hide
          Uma Maheswara Rao G added a comment -

          I am not so favor of adding directly static tools related annotations into code.
          In Hadoop projects(HDFS, Mapreduce) we are using this exclude-filter for invalid find bugs. So, I proposed this here.

          I think we can decide first how we will exclude the invalid bugs, then we can start working on the bug fix directly.

          Show
          Uma Maheswara Rao G added a comment - I am not so favor of adding directly static tools related annotations into code. In Hadoop projects(HDFS, Mapreduce) we are using this exclude-filter for invalid find bugs. So, I proposed this here. I think we can decide first how we will exclude the invalid bugs, then we can start working on the bug fix directly.
          Hide
          Uma Maheswara Rao G added a comment -

          Who ever wants to generate the findbugs html report locally, follow the below steps.

          1) add the below target in pom.xml. Already there is one transformationSet available in pom.xml. just we can place this after that transformationSet.

          <transformationSet>
          <dir>$

          {basedir}/target/</dir>
          <includes>
          <include>findbugsXml.xml</include>
          </includes>
          <stylesheet>E:/SoftWares/findbugs-1.3.9/findbugs-1.3.9/src/xsl/default.xsl</stylesheet>
          <outputDir>${basedir}

          /target/</outputDir>
          </transformationSet>

          2) Make sure to update the above findbugs xsl path correctly referring to your local path of findbugs.

          3) run 'mvn findbugs:findbugs'

          4) run 'mvn site'

          now $

          {basedir}/target/findbugsXml.xml will be replaced with html report. rename to ${basedir}

          /target/findbugsXml.html and open.

          Show
          Uma Maheswara Rao G added a comment - Who ever wants to generate the findbugs html report locally, follow the below steps. 1) add the below target in pom.xml. Already there is one transformationSet available in pom.xml. just we can place this after that transformationSet. <transformationSet> <dir>$ {basedir}/target/</dir> <includes> <include>findbugsXml.xml</include> </includes> <stylesheet>E:/SoftWares/findbugs-1.3.9/findbugs-1.3.9/src/xsl/default.xsl</stylesheet> <outputDir>${basedir} /target/</outputDir> </transformationSet> 2) Make sure to update the above findbugs xsl path correctly referring to your local path of findbugs. 3) run 'mvn findbugs:findbugs' 4) run 'mvn site' now $ {basedir}/target/findbugsXml.xml will be replaced with html report. rename to ${basedir} /target/findbugsXml.html and open.
          Hide
          Uma Maheswara Rao G added a comment -

          Also another case to use exclude filter is, we have many findbugs reported from generated code (from thrift).

          We can just add that package to the filter file.

          ex:

          <FindBugsFilter>
               <Match>
                  <Package name="org.apache.hadoop.hbase.thrift.generated" />
                </Match>
           </FindBugsFilter>
          
          Show
          Uma Maheswara Rao G added a comment - Also another case to use exclude filter is, we have many findbugs reported from generated code (from thrift). We can just add that package to the filter file. ex: <FindBugsFilter> <Match> <Package name= "org.apache.hadoop.hbase.thrift.generated" /> </Match> </FindBugsFilter>
          Hide
          Uma Maheswara Rao G added a comment -

          As a initial start I updated the patch with filter file.

          1) added filter file
          2) excluded generated packges.
          org.apache.hadoop.hbase.thrift2.generated, org.apache.hadoop.hbase.thrift.generated, org.apache.hadoop.hbase.rest.protobuf.generated

          This reduces the findbugs count from 772 to 601.

          [WARNING]
          [INFO]
          [INFO] ------------------------------------------------------------------------
          [INFO] Building HBase 0.95-SNAPSHOT
          [INFO] ------------------------------------------------------------------------
          [INFO]
          [INFO] — findbugs-maven-plugin:2.4.0:findbugs (default-cli) @ hbase —
          [INFO] Fork Value is true
          [java] Warnings generated: 601
          [INFO] Done FindBugs Analysis....
          [INFO] ------------------------------------------------------------------------
          [INFO] BUILD SUCCESS
          [INFO] ------------------------------------------------------------------------
          [INFO] Total time: 3:19.270s
          [INFO] Finished at: Tue Mar 27 03:56:20 IST 2012
          [INFO] Final Memory: 13M/55M
          [INFO] ------------------------------------------------------------------------

          note: currently we have already increased the count of findbugs in test-patch.properties. While verifying, I just reverted back to 0 for testing.

          Show
          Uma Maheswara Rao G added a comment - As a initial start I updated the patch with filter file. 1) added filter file 2) excluded generated packges. org.apache.hadoop.hbase.thrift2.generated, org.apache.hadoop.hbase.thrift.generated, org.apache.hadoop.hbase.rest.protobuf.generated This reduces the findbugs count from 772 to 601. [WARNING] [INFO] [INFO] ------------------------------------------------------------------------ [INFO] Building HBase 0.95-SNAPSHOT [INFO] ------------------------------------------------------------------------ [INFO] [INFO] — findbugs-maven-plugin:2.4.0:findbugs (default-cli) @ hbase — [INFO] Fork Value is true [java] Warnings generated: 601 [INFO] Done FindBugs Analysis.... [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS [INFO] ------------------------------------------------------------------------ [INFO] Total time: 3:19.270s [INFO] Finished at: Tue Mar 27 03:56:20 IST 2012 [INFO] Final Memory: 13M/55M [INFO] ------------------------------------------------------------------------ note: currently we have already increased the count of findbugs in test-patch.properties. While verifying, I just reverted back to 0 for testing.
          Hide
          stack added a comment -

          I'm good w/ this patch. We should add your instruction on how to gen the findbugs report to the release notes for this issue and to the reference guide (I can add it there if you add it to the release notes). I thought we were building the findbugs report when we generated the site but we are not doing this it seems. Findbugs is being generated by the hadoopqa builds.

          Show
          stack added a comment - I'm good w/ this patch. We should add your instruction on how to gen the findbugs report to the release notes for this issue and to the reference guide (I can add it there if you add it to the release notes). I thought we were building the findbugs report when we generated the site but we are not doing this it seems. Findbugs is being generated by the hadoopqa builds.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12520025/HBASE-5598.patch
          against trunk revision .

          +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 generated 13 javac compiler warnings (more than the trunk's current 5 warnings).

          +1 findbugs. The patch does not introduce any 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 unit tests:
          org.apache.hadoop.hbase.regionserver.TestSplitTransactionOnCluster

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1313//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1313//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/12520025/HBASE-5598.patch against trunk revision . +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 generated 13 javac compiler warnings (more than the trunk's current 5 warnings). +1 findbugs. The patch does not introduce any 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 unit tests: org.apache.hadoop.hbase.regionserver.TestSplitTransactionOnCluster Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1313//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1313//console This message is automatically generated.
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Jon
          Can you add the subtasks here that you have identified as the categories of findbugs.
          We have some tasks started over here. We can take up few in that.

          Show
          ramkrishna.s.vasudevan added a comment - @Jon Can you add the subtasks here that you have identified as the categories of findbugs. We have some tasks started over here. We can take up few in that.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12520103/findbugs-gui-report.jpg
          against trunk revision .

          +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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1320//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/12520103/findbugs-gui-report.jpg against trunk revision . +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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1320//console This message is automatically generated.
          Hide
          Uma Maheswara Rao G added a comment -

          @Stack, Thanks a lot for taking a look.

          Actually I have given the steps which i followed for generating the findbugs locally in comment https://issues.apache.org/jira/browse/HBASE-5598?focusedCommentId=13238820&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13238820

          Also I found better command to view the findbugs in UI directly.
          This command would be better to update in notes.

          E:\Repositories\Hbase>mvn findbugs:gui

          This is giving all the findbug comments by reffering code snippets directly. This should help the developers to generate the reports in their envs.
          Also attached the sample screenshot of the report.

          Show
          Uma Maheswara Rao G added a comment - @Stack, Thanks a lot for taking a look. Actually I have given the steps which i followed for generating the findbugs locally in comment https://issues.apache.org/jira/browse/HBASE-5598?focusedCommentId=13238820&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13238820 Also I found better command to view the findbugs in UI directly. This command would be better to update in notes. E:\Repositories\Hbase>mvn findbugs:gui This is giving all the findbug comments by reffering code snippets directly. This should help the developers to generate the reports in their envs. Also attached the sample screenshot of the report.
          Hide
          Jonathan Hsieh added a comment -

          Uma, good stuff. Please take a look at this thread from the mailing list if you haven't seen it yet.

          http://mail-archives.apache.org/mod_mbox/hbase-dev/201203.mbox/browser

          I'd suggest updating the findbugs warning count in dev-support/test-patch.properties. Mind moving it over to HBASE-5642? We can keep this on as the umbrella issue.

          For the other folks interested, here's some link on how to add findbugs/findbugs excludes for those who want to jump on some issues.

          http://findbugs.sourceforge.net/manual/filter.html
          http://mojo.codehaus.org/findbugs-maven-plugin-2.4.0/

          You can run findbugs locally via 'mvn findbugs:findbugs'. I thought we auto gen'ed the file for the website – making findbugs run and generate html when 'mvn site' is run might be another subtask to add.

          Show
          Jonathan Hsieh added a comment - Uma, good stuff. Please take a look at this thread from the mailing list if you haven't seen it yet. http://mail-archives.apache.org/mod_mbox/hbase-dev/201203.mbox/browser I'd suggest updating the findbugs warning count in dev-support/test-patch.properties. Mind moving it over to HBASE-5642 ? We can keep this on as the umbrella issue. For the other folks interested, here's some link on how to add findbugs/findbugs excludes for those who want to jump on some issues. http://findbugs.sourceforge.net/manual/filter.html http://mojo.codehaus.org/findbugs-maven-plugin-2.4.0/ You can run findbugs locally via 'mvn findbugs:findbugs'. I thought we auto gen'ed the file for the website – making findbugs run and generate html when 'mvn site' is run might be another subtask to add.
          Hide
          Uma Maheswara Rao G added a comment -

          Uma, good stuff. Please take a look at this thread from the mailing list if you haven't seen it yet.

          http://mail-archives.apache.org/mod_mbox/hbase-dev/201203.mbox/browser

          Thanks Jon. Yes, I have seen that discussion in mailing lists.

          I'd suggest updating the findbugs warning count in dev-support/test-patch.properties. Mind moving it over to HBASE-5642? We can keep this on as the umbrella is

          Sure, I will update the patch in HBASE-5642. But regarding updating the count, I am not agreeing. I am planning to use exclude filter file to exclude the invalid bugs. Please see the discussion in HBASE-5597 about updating the count. Please correct me if i understood wrongly your question. Thanks a lot.

          You can run findbugs locally via 'mvn findbugs:findbugs'. I thought we auto gen'ed the file for the website – making findbugs run and generate html when 'mvn site'

          Alternatively you can also use 'mvn findbugs:gui'. This command will generate the report in UI. Updated the samplke report in this JIRA.

          Thanks
          Uma

          Show
          Uma Maheswara Rao G added a comment - Uma, good stuff. Please take a look at this thread from the mailing list if you haven't seen it yet. http://mail-archives.apache.org/mod_mbox/hbase-dev/201203.mbox/browser Thanks Jon. Yes, I have seen that discussion in mailing lists. I'd suggest updating the findbugs warning count in dev-support/test-patch.properties. Mind moving it over to HBASE-5642 ? We can keep this on as the umbrella is Sure, I will update the patch in HBASE-5642 . But regarding updating the count, I am not agreeing. I am planning to use exclude filter file to exclude the invalid bugs. Please see the discussion in HBASE-5597 about updating the count. Please correct me if i understood wrongly your question. Thanks a lot. You can run findbugs locally via 'mvn findbugs:findbugs'. I thought we auto gen'ed the file for the website – making findbugs run and generate html when 'mvn site' Alternatively you can also use 'mvn findbugs:gui'. This command will generate the report in UI. Updated the samplke report in this JIRA. Thanks Uma
          Hide
          Jonathan Hsieh added a comment -

          Sure, I will update the patch in HBASE-5642. But regarding updating the count, I am not agreeing. I am planning to use exclude filter file to exclude the invalid bugs. Please see the discussion in HBASE-5597 about updating the count. Please correct me if i understood wrongly your question. Thanks a lot.

          Here's what I'm suggesting. Eventually we set the number to 0 due to the excludes files. Meanwhile, as we commit exclude/fix these warnings patches, we update the count to the number you expect to see after the patch. Patches in flight would just see the same number of new warnings it introduced instead of warnings from disappearing as you mentioned in HBASE-5597.

          Show
          Jonathan Hsieh added a comment - Sure, I will update the patch in HBASE-5642 . But regarding updating the count, I am not agreeing. I am planning to use exclude filter file to exclude the invalid bugs. Please see the discussion in HBASE-5597 about updating the count. Please correct me if i understood wrongly your question. Thanks a lot. Here's what I'm suggesting. Eventually we set the number to 0 due to the excludes files. Meanwhile, as we commit exclude/fix these warnings patches, we update the count to the number you expect to see after the patch. Patches in flight would just see the same number of new warnings it introduced instead of warnings from disappearing as you mentioned in HBASE-5597 .
          Hide
          Uma Maheswara Rao G added a comment -

          Thanks a lot, Jon. I got your point. Make sense to me. I will update the patch accordingly in HBASE-5642.

          Show
          Uma Maheswara Rao G added a comment - Thanks a lot, Jon. I got your point. Make sense to me. I will update the patch accordingly in HBASE-5642 .
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2702 (See https://builds.apache.org/job/HBase-TRUNK/2702/)
          HBASE-5598 [findbugs] Exclude Protobuf warnings from wire compat patches (Revision 1307656)

          Result = SUCCESS
          jmhsieh :
          Files :

          • /hbase/trunk/dev-support/findbugs-exclude.xml
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2702 (See https://builds.apache.org/job/HBase-TRUNK/2702/ ) HBASE-5598 [findbugs] Exclude Protobuf warnings from wire compat patches (Revision 1307656) Result = SUCCESS jmhsieh : Files : /hbase/trunk/dev-support/findbugs-exclude.xml
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #155 (See https://builds.apache.org/job/HBase-TRUNK-security/155/)
          HBASE-5598 [findbugs] Exclude Protobuf warnings from wire compat patches (Revision 1307656)

          Result = SUCCESS
          jmhsieh :
          Files :

          • /hbase/trunk/dev-support/findbugs-exclude.xml
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #155 (See https://builds.apache.org/job/HBase-TRUNK-security/155/ ) HBASE-5598 [findbugs] Exclude Protobuf warnings from wire compat patches (Revision 1307656) Result = SUCCESS jmhsieh : Files : /hbase/trunk/dev-support/findbugs-exclude.xml
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4612/
          -----------------------------------------------------------

          Review request for hbase.

          Summary
          -------

          Fixes for issues reported in findbugs for "Correctness" category.

          This addresses bug HBASE-5598.
          https://issues.apache.org/jira/browse/HBASE-5598

          Diffs


          dev-support/test-patch.properties 2209d27
          src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204
          src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java 37bcaba
          src/main/java/org/apache/hadoop/hbase/mapreduce/CellCounter.java 32d66fb
          src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15
          src/main/java/org/apache/hadoop/hbase/master/DefaultLoadBalancer.java 1676832
          src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace
          src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 3515d4a
          src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 8ae60a3
          src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreLAB.java 2694897
          src/main/java/org/apache/hadoop/hbase/regionserver/ShutdownHook.java e3b230e
          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 0c7b396
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java 2e98b39
          src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 698bb3d
          src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 8950c9f
          src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac
          src/main/java/org/apache/hadoop/hbase/util/FSUtils.java aebe5b0
          src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java c21377c
          src/main/java/org/apache/hadoop/hbase/util/Merge.java 04f15d4
          src/main/java/org/apache/hadoop/hbase/util/PoolMap.java 8e2a856

          Diff: https://reviews.apache.org/r/4612/diff

          Testing
          -------

          Ran test-patch.sh and unit tests.

          Thanks,

          David

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4612/ ----------------------------------------------------------- Review request for hbase. Summary ------- Fixes for issues reported in findbugs for "Correctness" category. This addresses bug HBASE-5598 . https://issues.apache.org/jira/browse/HBASE-5598 Diffs dev-support/test-patch.properties 2209d27 src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon ae76204 src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java 37bcaba src/main/java/org/apache/hadoop/hbase/mapreduce/CellCounter.java 32d66fb src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15 src/main/java/org/apache/hadoop/hbase/master/DefaultLoadBalancer.java 1676832 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 3515d4a src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 8ae60a3 src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreLAB.java 2694897 src/main/java/org/apache/hadoop/hbase/regionserver/ShutdownHook.java e3b230e src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 0c7b396 src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java 2e98b39 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 698bb3d src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 8950c9f src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac src/main/java/org/apache/hadoop/hbase/util/FSUtils.java aebe5b0 src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java c21377c src/main/java/org/apache/hadoop/hbase/util/Merge.java 04f15d4 src/main/java/org/apache/hadoop/hbase/util/PoolMap.java 8e2a856 Diff: https://reviews.apache.org/r/4612/diff Testing ------- Ran test-patch.sh and unit tests. Thanks, David
          Hide
          stack added a comment -

          As Jon suggests out on the list, lets kick this issue and get this effort going again.

          Show
          stack added a comment - As Jon suggests out on the list, lets kick this issue and get this effort going again.
          Hide
          stack added a comment -

          Marking critical so gets a bit of attention.

          Show
          stack added a comment - Marking critical so gets a bit of attention.
          Hide
          Nicolas Liochon added a comment -

          part1 contains:

          • migration to maven plugin v2.5.2
          • some exclusions
          • some trivial fixes

          It works locally, so I will commit it tomorrow if there is no objections.

          Show
          Nicolas Liochon added a comment - part1 contains: migration to maven plugin v2.5.2 some exclusions some trivial fixes It works locally, so I will commit it tomorrow if there is no objections.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12562225/5598.part1.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          +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 (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 unit tests:
          org.apache.hadoop.hbase.replication.TestReplication
          org.apache.hadoop.hbase.replication.TestReplicationWithCompression

          -1 core zombie tests. There are zombie tests. See build logs for details.

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//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/12562225/5598.part1.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +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 (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 unit tests: org.apache.hadoop.hbase.replication.TestReplication org.apache.hadoop.hbase.replication.TestReplicationWithCompression -1 core zombie tests . There are zombie tests. See build logs for details. Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3669//console This message is automatically generated.
          Hide
          stack added a comment -

          Looks good Nicolas Liochon. I do not see corresponding edit of dev-tools/test-patch.properties changing

          21 OK_FINDBUGS_WARNINGS=517

          to a new value. Otherwise, looks great.

          Show
          stack added a comment - Looks good Nicolas Liochon . I do not see corresponding edit of dev-tools/test-patch.properties changing 21 OK_FINDBUGS_WARNINGS=517 to a new value. Otherwise, looks great.
          Hide
          Nicolas Liochon added a comment -

          Thanks for the review, Stack. Committed.

          On commit, I updated the value. 226 left.

          Show
          Nicolas Liochon added a comment - Thanks for the review, Stack. Committed. On commit, I updated the value. 226 left.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #3652 (See https://builds.apache.org/job/HBase-TRUNK/3652/)
          HBASE-5598 Analyse and fix the findbugs reporting by QA and add invalid bugs into findbugs-excludeFilter file (Revision 1425351)

          Result = FAILURE
          nkeywal :
          Files :

          • /hbase/trunk/dev-support/findbugs-exclude.xml
          • /hbase/trunk/dev-support/test-patch.properties
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java
          • /hbase/trunk/pom.xml
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #3652 (See https://builds.apache.org/job/HBase-TRUNK/3652/ ) HBASE-5598 Analyse and fix the findbugs reporting by QA and add invalid bugs into findbugs-excludeFilter file (Revision 1425351) Result = FAILURE nkeywal : Files : /hbase/trunk/dev-support/findbugs-exclude.xml /hbase/trunk/dev-support/test-patch.properties /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java /hbase/trunk/pom.xml
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #310 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/310/)
          HBASE-5598 Analyse and fix the findbugs reporting by QA and add invalid bugs into findbugs-excludeFilter file (Revision 1425351)

          Result = FAILURE
          nkeywal :
          Files :

          • /hbase/trunk/dev-support/findbugs-exclude.xml
          • /hbase/trunk/dev-support/test-patch.properties
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java
          • /hbase/trunk/pom.xml
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #310 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/310/ ) HBASE-5598 Analyse and fix the findbugs reporting by QA and add invalid bugs into findbugs-excludeFilter file (Revision 1425351) Result = FAILURE nkeywal : Files : /hbase/trunk/dev-support/findbugs-exclude.xml /hbase/trunk/dev-support/test-patch.properties /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java /hbase/trunk/pom.xml
          Hide
          Nicolas Liochon added a comment -

          Note it's also possible to have tags in the code to suppress a warning: http://stackoverflow.com/questions/249536/when-using-eclipse-with-findbugs-can-you-mark-a-bug-as-not-a-bug-and-have-it-rem

          @edu.umd.cs.findbugs.annotations.SuppressWarnings(
              value="EQ_COMPARETO_USE_OBJECT_EQUALS", 
              justification="because I know better")
          

          It's not the standard SuppressWarnings: it requires to link with findbugs jars.

          Show
          Nicolas Liochon added a comment - Note it's also possible to have tags in the code to suppress a warning: http://stackoverflow.com/questions/249536/when-using-eclipse-with-findbugs-can-you-mark-a-bug-as-not-a-bug-and-have-it-rem @edu.umd.cs.findbugs.annotations.SuppressWarnings( value= "EQ_COMPARETO_USE_OBJECT_EQUALS" , justification= "because I know better" ) It's not the standard SuppressWarnings: it requires to link with findbugs jars.
          Hide
          Nicolas Liochon added a comment -

          part2:

          • adds the dependency to findbugs for the annotations. It's a compile only dependency. License is LGPL.
          • suppress a few not critical warnings
          • fixes a few others

          Locally, I'm now at 144.

          Show
          Nicolas Liochon added a comment - part2: adds the dependency to findbugs for the annotations. It's a compile only dependency. License is LGPL. suppress a few not critical warnings fixes a few others Locally, I'm now at 144.
          Hide
          stack added a comment -

          @nkeywal Do you want to do part2 in another issue linked to from here since we have already committed a patch against this issue?

          Why move the defines in HRegionInfo?

          This is a nice trick:

          +    @edu.umd.cs.findbugs.annotations.SuppressWarnings (value="ES_COMPARING_STRINGS_WITH_EQ",
          +        justification="Optimization")
          

          What warning do we suppress by doing this:

          • return new StringBuilder("AuthResult")
          • .append(toContextString()).toString();
            + return "AuthResult" + toContextString();

          I know its not 'important' but these kinda findings are good:

          • StringBuffer sb = new StringBuffer("");
            + StringBuilder sb = new StringBuilder("");

          We are going back to findbugs 2.0.1 from 2.5.2?

          Is this define used anywhere: findbugs-maven-plugin.version

          Good work Nicolas

          Show
          stack added a comment - @nkeywal Do you want to do part2 in another issue linked to from here since we have already committed a patch against this issue? Why move the defines in HRegionInfo? This is a nice trick: + @edu.umd.cs.findbugs.annotations.SuppressWarnings (value= "ES_COMPARING_STRINGS_WITH_EQ" , + justification= "Optimization" ) What warning do we suppress by doing this: return new StringBuilder("AuthResult") .append(toContextString()).toString(); + return "AuthResult" + toContextString(); I know its not 'important' but these kinda findings are good: StringBuffer sb = new StringBuffer(""); + StringBuilder sb = new StringBuilder(""); We are going back to findbugs 2.0.1 from 2.5.2? Is this define used anywhere: findbugs-maven-plugin.version Good work Nicolas
          Hide
          Nicolas Liochon added a comment -

          @nkeywal Do you want to do part2 in another issue linked to from here since we have already committed a patch against this issue?

          Ok, I will do that.

          Why move the defines in HRegionInfo?

          We're constructing an object while the static fields are not yet all initialized:

          class A{
           static A = new A();
           static int i = 15;
           A(){ print "i=" + i; }
          }
          

          prints i=0;

          Tricky isn't it?

          return new StringBuilder("AuthResult")

          .append(toContextString()).toString();
          + return "AuthResult" + toContextString();

          Don't remember its name (it could be an IntelliJ warning as well, I fix both), but it's just that the new code is simpler and at least as efficient as the previous one, as "AuthResult" is a constant.

          We are going back to findbugs 2.0.1 from 2.5.2?

          There are two versions, one for findbugs, one for the maven plugin. Maven plugin 2.5.2 includes findbugs 2.0.1.
          The naming was previously confusing, so I renamed the variables.
          And we need to explicitly define the two versions, as we're including the plugin but as well the annotations.
          Last findbugs is 2.0.2, but I put 2.0.1 to match the version included by the maven plugin

          Is this define used anywhere: findbugs-maven-plugin.version

          Yep, in the main pom, when including the maven plugin.

          Show
          Nicolas Liochon added a comment - @nkeywal Do you want to do part2 in another issue linked to from here since we have already committed a patch against this issue? Ok, I will do that. Why move the defines in HRegionInfo? We're constructing an object while the static fields are not yet all initialized: class A{ static A = new A(); static int i = 15; A(){ print "i=" + i; } } prints i=0; Tricky isn't it? return new StringBuilder("AuthResult") .append(toContextString()).toString(); + return "AuthResult" + toContextString(); Don't remember its name (it could be an IntelliJ warning as well, I fix both), but it's just that the new code is simpler and at least as efficient as the previous one, as "AuthResult" is a constant. We are going back to findbugs 2.0.1 from 2.5.2? There are two versions, one for findbugs, one for the maven plugin. Maven plugin 2.5.2 includes findbugs 2.0.1. The naming was previously confusing, so I renamed the variables. And we need to explicitly define the two versions, as we're including the plugin but as well the annotations. Last findbugs is 2.0.2, but I put 2.0.1 to match the version included by the maven plugin Is this define used anywhere: findbugs-maven-plugin.version Yep, in the main pom, when including the maven plugin.
          Hide
          Ted Yu added a comment -

          Nicolas Liochon:
          This JIRA seems to overlap with your recent work. Can we close this or lower its priority ?

          Show
          Ted Yu added a comment - Nicolas Liochon : This JIRA seems to overlap with your recent work. Can we close this or lower its priority ?
          Hide
          stack added a comment -

          Closing.

          We are down to 112 in hbase-server module now (most other modules are at zero) so we are ahead of the last reported 144 that @nkeywal cites. nkeywal has also updated the refguide so devs can figure how to fix findbugs themselves and that seems to be working out.

          Closing as no done because this is an umbrella issue that got us most of the way toward zero findbugs (still some work but can be done as part of normal patch making now findbugs consciousness has been successfully raised)

          Show
          stack added a comment - Closing. We are down to 112 in hbase-server module now (most other modules are at zero) so we are ahead of the last reported 144 that @nkeywal cites. nkeywal has also updated the refguide so devs can figure how to fix findbugs themselves and that seems to be working out. Closing as no done because this is an umbrella issue that got us most of the way toward zero findbugs (still some work but can be done as part of normal patch making now findbugs consciousness has been successfully raised)
          Hide
          stack added a comment -

          Marking closed.

          Show
          stack added a comment - Marking closed.

            People

            • Assignee:
              Uma Maheswara Rao G
              Reporter:
              Uma Maheswara Rao G
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development