Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.95.0
    • Component/s: scripts
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    1. HBASE-5652-v1.patch
      5 kB
      Gregory Chanan
    2. HBASE-5652-v0.patch
      5 kB
      Gregory Chanan

      Activity

      Hide
      stack added a comment -

      Marking closed.

      Show
      stack added a comment - Marking closed.
      Hide
      Hudson added a comment -

      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
      Show
      Hudson added a comment - 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
      Hide
      Hudson added a comment -

      Integrated in HBase-TRUNK #2816 (See https://builds.apache.org/job/HBase-TRUNK/2816/)
      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
      Show
      Hudson added a comment - Integrated in HBase-TRUNK #2816 (See https://builds.apache.org/job/HBase-TRUNK/2816/ ) 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
      Hide
      Jonathan Hsieh added a comment -

      Thanks Greg! Committed to trunk with an updated bug count of 517.

      Show
      Jonathan Hsieh added a comment - Thanks Greg! Committed to trunk with an updated bug count of 517.
      Hide
      stack added a comment -

      I ran TestSplitLogManager ten times on trunk and it passed each time. FYI.

      Show
      stack added a comment - I ran TestSplitLogManager ten times on trunk and it passed each time. FYI.
      Hide
      Hadoop QA added a comment -

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

      Show
      Hadoop QA added a comment - -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.
      Hide
      Gregory Chanan added a comment -

      Fixed spacing as requested by Ted.
      Haven't updated test-patch.properties, as I won't know what value to use until Hadoop QA runs.

      Show
      Gregory Chanan added a comment - Attached HBASE-5652 -v1.patch * Fixed spacing as requested by Ted. Haven't updated test-patch.properties, as I won't know what value to use until Hadoop QA runs.
      Hide
      Jonathan Hsieh added a comment -

      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.

      Show
      Jonathan Hsieh added a comment - 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.
      Hide
      Ted Yu added a comment -

      I think we can release this.cacheFlushLock before setting this.logRollRunning to false.
      We don't need the extra try/finally construct.

      Show
      Ted Yu added a comment - I think we can release this.cacheFlushLock before setting this.logRollRunning to false. We don't need the extra try/finally construct.
      Hide
      Gregory Chanan added a comment -

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

      Show
      Gregory Chanan added a comment - @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.
      Hide
      Uma Maheswara Rao G added a comment -

      @Gregory, Also please update the test-patch.properties reflecting to the current findbugs OK count with your patch

      Show
      Uma Maheswara Rao G added a comment - @Gregory, Also please update the test-patch.properties reflecting to the current findbugs OK count with your patch
      Hide
      ramkrishna.s.vasudevan added a comment -

      Yes Uma you are right. Jon also is strict in moving things to exclude file.

      Show
      ramkrishna.s.vasudevan added a comment - Yes Uma you are right. Jon also is strict in moving things to exclude file.
      Hide
      Uma Maheswara Rao G added a comment -

      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.

      try {
      +        this.logRollRunning = false;
      +      } finally {
      +        this.cacheFlushLock.unlock();
      +      }
      

      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.

      Show
      Uma Maheswara Rao G added a comment - 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. try { + this .logRollRunning = false ; + } finally { + this .cacheFlushLock.unlock(); + } 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.
      Hide
      ramkrishna.s.vasudevan added a comment -

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

      Show
      ramkrishna.s.vasudevan added a comment - @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.
      Hide
      Ted Yu added a comment -
      +        if(status != null) status.cleanup();
      

      Please insert space between if and (.

      +      try {
      +        this.logRollRunning = false;
      +      } finally {
      +        this.cacheFlushLock.unlock();
      +      }
      

      The assignment wouldn't throw exception. Is the above try block needed ?

      Show
      Ted Yu added a comment - + if (status != null ) status.cleanup(); Please insert space between if and (. + try { + this .logRollRunning = false ; + } finally { + this .cacheFlushLock.unlock(); + } The assignment wouldn't throw exception. Is the above try block needed ?
      Hide
      Hadoop QA added a comment -

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

      Show
      Hadoop QA added a comment - -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.
      Hide
      Gregory Chanan added a comment -

      Attached patch that handles all the UL issues except two, which are on the exception list (I haven't tested the exception list yet, we'll see if it works in Hadoop QA).

      Those two are startRegionOperation and startBulkRegionOperation, which are correct (they release the lock and throw an exception if the region is closed, otherwise, hold the lock and continue processing.

      Show
      Gregory Chanan added a comment - Attacked HBASE-5652 -v0.patch * Attached patch that handles all the UL issues except two, which are on the exception list (I haven't tested the exception list yet, we'll see if it works in Hadoop QA). Those two are startRegionOperation and startBulkRegionOperation, which are correct (they release the lock and throw an exception if the region is closed, otherwise, hold the lock and continue processing.

        People

        • Assignee:
          Gregory Chanan
          Reporter:
          Jonathan Hsieh
        • Votes:
          0 Vote for this issue
          Watchers:
          3 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development