[jira] [Commented] (HBASE-5652) [findbugs] Fix lock release on all paths
[ https://issues.apache.org/jira/browse/HBASE-5652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13262394#comment-13262394 ] Hudson commented on HBASE-5652: --- Integrated in HBase-TRUNK-security #185 (See [https://builds.apache.org/job/HBase-TRUNK-security/185/]) HBASE-5652 [findbugs] Fix lock release on all paths (Gregory Channan) (Revision 1330628) Result = SUCCESS jmhsieh : Files : * /hbase/trunk/dev-support/findbugs-exclude.xml * /hbase/trunk/dev-support/test-patch.properties * /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java * /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/LogRoller.java * /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java * /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java [findbugs] Fix lock release on all paths - Key: HBASE-5652 URL: https://issues.apache.org/jira/browse/HBASE-5652 Project: HBase Issue Type: Sub-task Components: scripts Reporter: Jonathan Hsieh Assignee: Gregory Chanan Fix For: 0.96.0 Attachments: HBASE-5652-v0.patch, HBASE-5652-v1.patch See https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html#Warnings_MT_CORRECTNESS Category UL -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5652) [findbugs] Fix lock release on all paths
[ https://issues.apache.org/jira/browse/HBASE-5652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13259439#comment-13259439 ] Jonathan Hsieh commented on HBASE-5652: --- I side with Greg here. I'd rather we only add excludes for things that *must* be put there. In that particular case, we have readable and reasonable code that does not require the exclude. [findbugs] Fix lock release on all paths - Key: HBASE-5652 URL: https://issues.apache.org/jira/browse/HBASE-5652 Project: HBase Issue Type: Sub-task Components: scripts Reporter: Jonathan Hsieh Assignee: Gregory Chanan Attachments: HBASE-5652-v0.patch See https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html#Warnings_MT_CORRECTNESS Category UL -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5652) [findbugs] Fix lock release on all paths
[ https://issues.apache.org/jira/browse/HBASE-5652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13259946#comment-13259946 ] Hadoop QA commented on HBASE-5652: -- -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12523859/HBASE-5652-v1.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 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.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1614//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1614//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1614//console This message is automatically generated. [findbugs] Fix lock release on all paths - Key: HBASE-5652 URL: https://issues.apache.org/jira/browse/HBASE-5652 Project: HBase Issue Type: Sub-task Components: scripts Reporter: Jonathan Hsieh Assignee: Gregory Chanan Attachments: HBASE-5652-v0.patch, HBASE-5652-v1.patch See https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html#Warnings_MT_CORRECTNESS Category UL -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5652) [findbugs] Fix lock release on all paths
[ https://issues.apache.org/jira/browse/HBASE-5652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13259957#comment-13259957 ] stack commented on HBASE-5652: -- I ran TestSplitLogManager ten times on trunk and it passed each time. FYI. [findbugs] Fix lock release on all paths - Key: HBASE-5652 URL: https://issues.apache.org/jira/browse/HBASE-5652 Project: HBase Issue Type: Sub-task Components: scripts Reporter: Jonathan Hsieh Assignee: Gregory Chanan Attachments: HBASE-5652-v0.patch, HBASE-5652-v1.patch See https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html#Warnings_MT_CORRECTNESS Category UL -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5652) [findbugs] Fix lock release on all paths
[ https://issues.apache.org/jira/browse/HBASE-5652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13259177#comment-13259177 ] Uma Maheswara Rao G commented on HBASE-5652: @Gregory, Also please update the test-patch.properties reflecting to the current findbugs OK count with your patch [findbugs] Fix lock release on all paths - Key: HBASE-5652 URL: https://issues.apache.org/jira/browse/HBASE-5652 Project: HBase Issue Type: Sub-task Components: scripts Reporter: Jonathan Hsieh Assignee: Gregory Chanan Attachments: HBASE-5652-v0.patch See https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html#Warnings_MT_CORRECTNESS Category UL -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5652) [findbugs] Fix lock release on all paths
[ https://issues.apache.org/jira/browse/HBASE-5652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13259272#comment-13259272 ] Gregory Chanan commented on HBASE-5652: --- @Ted: will update spacing @Uma: will update test-patch.properties On the assignment not being able to throw an exception, I agree. However it seems better to put it into a try/finally because: 1) it keeps the pattern the same with other usages 2) it works even if the line changes. Imagine if we made it an exclude and then the line changed to something that could throw an exception. We wouldn't see the warning because the exclude would hide it. 3) as noted by Uma, exceptions are fragile, e.g. if you rename the function you also have to change the exclude. [findbugs] Fix lock release on all paths - Key: HBASE-5652 URL: https://issues.apache.org/jira/browse/HBASE-5652 Project: HBase Issue Type: Sub-task Components: scripts Reporter: Jonathan Hsieh Assignee: Gregory Chanan Attachments: HBASE-5652-v0.patch See https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html#Warnings_MT_CORRECTNESS Category UL -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5652) [findbugs] Fix lock release on all paths
[ https://issues.apache.org/jira/browse/HBASE-5652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13259277#comment-13259277 ] Zhihong Yu commented on HBASE-5652: --- I think we can release this.cacheFlushLock before setting this.logRollRunning to false. We don't need the extra try/finally construct. [findbugs] Fix lock release on all paths - Key: HBASE-5652 URL: https://issues.apache.org/jira/browse/HBASE-5652 Project: HBase Issue Type: Sub-task Components: scripts Reporter: Jonathan Hsieh Assignee: Gregory Chanan Attachments: HBASE-5652-v0.patch See https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html#Warnings_MT_CORRECTNESS Category UL -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5652) [findbugs] Fix lock release on all paths
[ https://issues.apache.org/jira/browse/HBASE-5652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13258786#comment-13258786 ] ramkrishna.s.vasudevan commented on HBASE-5652: --- @Greg I think the lock paths here are unlocked in finally block. And the try catch block added may not be needed. If Jon accepts we can add it into exclude file i think. @Jon Your thoughts please. [findbugs] Fix lock release on all paths - Key: HBASE-5652 URL: https://issues.apache.org/jira/browse/HBASE-5652 Project: HBase Issue Type: Sub-task Components: scripts Reporter: Jonathan Hsieh Assignee: Gregory Chanan Attachments: HBASE-5652-v0.patch See https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html#Warnings_MT_CORRECTNESS Category UL -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5652) [findbugs] Fix lock release on all paths
[
https://issues.apache.org/jira/browse/HBASE-5652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13258791#comment-13258791
]
Uma Maheswara Rao G commented on HBASE-5652:
Agreed with Ram, variable assignment in finally block before unlocking would
not cause any exception here. But the standard pattern for read/write locks I
have seen is, after acquiring the lock, every line should be in try and in
finally block we will release the lock. That might be the findbugs worry here.
But in this case, there is no way of throwing exception from variable
assignments. So, we can just skip I feel. Let's see Jon opinion on this.
Here try/finally almost no use.
{code}
try {
+this.logRollRunning = false;
+ } finally {
+this.cacheFlushLock.unlock();
+ }
{code}
Other problem I see in adding into exclude list is, we are not able to pin
point exact lication of the code. We may give just
package/class/method/feilds..and bug pattern,type ...etc. Unfortunately if same
bug introduces but this is valid to fix in the same area of code, then it may
get skipped due to other exclude entry presents in the file which is almost
matching to the same. So, we have to reduce exclude filter entries also as less
as possible.
[findbugs] Fix lock release on all paths
-
Key: HBASE-5652
URL: https://issues.apache.org/jira/browse/HBASE-5652
Project: HBase
Issue Type: Sub-task
Components: scripts
Reporter: Jonathan Hsieh
Assignee: Gregory Chanan
Attachments: HBASE-5652-v0.patch
See
https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html#Warnings_MT_CORRECTNESS
Category UL
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5652) [findbugs] Fix lock release on all paths
[ https://issues.apache.org/jira/browse/HBASE-5652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13258794#comment-13258794 ] ramkrishna.s.vasudevan commented on HBASE-5652: --- Yes Uma you are right. Jon also is strict in moving things to exclude file. [findbugs] Fix lock release on all paths - Key: HBASE-5652 URL: https://issues.apache.org/jira/browse/HBASE-5652 Project: HBase Issue Type: Sub-task Components: scripts Reporter: Jonathan Hsieh Assignee: Gregory Chanan Attachments: HBASE-5652-v0.patch See https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html#Warnings_MT_CORRECTNESS Category UL -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5652) [findbugs] Fix lock release on all paths
[ https://issues.apache.org/jira/browse/HBASE-5652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13258698#comment-13258698 ] Hadoop QA commented on HBASE-5652: -- -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12523581/HBASE-5652-v0.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 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.regionserver.TestSplitTransactionOnCluster Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1596//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1596//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1596//console This message is automatically generated. [findbugs] Fix lock release on all paths - Key: HBASE-5652 URL: https://issues.apache.org/jira/browse/HBASE-5652 Project: HBase Issue Type: Sub-task Components: scripts Reporter: Jonathan Hsieh Assignee: Gregory Chanan Attachments: HBASE-5652-v0.patch See https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html#Warnings_MT_CORRECTNESS Category UL -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (HBASE-5652) [findbugs] Fix lock release on all paths
[
https://issues.apache.org/jira/browse/HBASE-5652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13258763#comment-13258763
]
Zhihong Yu commented on HBASE-5652:
---
{code}
+if(status != null) status.cleanup();
{code}
Please insert space between if and (.
{code}
+ try {
+this.logRollRunning = false;
+ } finally {
+this.cacheFlushLock.unlock();
+ }
{code}
The assignment wouldn't throw exception. Is the above try block needed ?
[findbugs] Fix lock release on all paths
-
Key: HBASE-5652
URL: https://issues.apache.org/jira/browse/HBASE-5652
Project: HBase
Issue Type: Sub-task
Components: scripts
Reporter: Jonathan Hsieh
Assignee: Gregory Chanan
Attachments: HBASE-5652-v0.patch
See
https://builds.apache.org/job/PreCommit-HBASE-Build/1313//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html#Warnings_MT_CORRECTNESS
Category UL
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
