Details

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.

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.

Jonathan Hsieh
added a comment - 24/Mar/12 16:00 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?

+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.

Lars Hofhansl
added a comment - 24/Mar/12 21:26 +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.

Uma Maheswara Rao G
added a comment - 26/Mar/12 20:50 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.

Uma Maheswara Rao G
added a comment - 26/Mar/12 20:58 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.

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.

stack
added a comment - 27/Mar/12 00:09 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.

-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.

Hadoop QA
added a comment - 27/Mar/12 00:10 -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.

ramkrishna.s.vasudevan
added a comment - 27/Mar/12 04:29 @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.

-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.

Hadoop QA
added a comment - 27/Mar/12 11:22 -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.

Uma Maheswara Rao G
added a comment - 27/Mar/12 11:24 @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.

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.

Jonathan Hsieh
added a comment - 27/Mar/12 14:20 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.

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.

Uma Maheswara Rao G
added a comment - 27/Mar/12 14:38
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

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.

Jonathan Hsieh
added a comment - 27/Mar/12 17:01
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 .

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

Nicolas Liochon
added a comment - 22/Dec/12 12:50 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.

stack
added a comment - 22/Dec/12 21:41 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.

Nicolas Liochon
added a comment - 27/Dec/12 15:55 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.

stack
added a comment - 27/Dec/12 16:42 @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

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

Nicolas Liochon
added a comment - 27/Dec/12 17:16 @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.

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)

stack
added a comment - 25/Feb/13 19:54 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)