Uploaded image for project: 'HBase'
  1. HBase
  2. HBASE-6002

Possible chance of resource leak in HlogSplitter

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.94.0, 0.95.2
    • Fix Version/s: 0.94.1, 0.95.0
    • Component/s: wal
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      In HLogSplitter.splitLogFileToTemp-Reader(in) is not closed and in finally block in loop while closing the writers(wap.w) if any exception comes other writers won't close.

      1. HBASE-6002.patch
        2 kB
        Chinna Rao Lalam
      2. HBASE-6002_0.94_1.patch
        2 kB
        Chinna Rao Lalam
      3. HBASE-6002_trunk.patch
        2 kB
        Chinna Rao Lalam
      4. HBASE-6002_trunk_1.patch
        6 kB
        ramkrishna.s.vasudevan
      5. HBASE-6002_0.92_1.patch
        5 kB
        ramkrishna.s.vasudevan
      6. HBASE-6002_0.94_1.patch
        5 kB
        ramkrishna.s.vasudevan
      7. HBASE-6002_trunk_2.patch
        6 kB
        ramkrishna.s.vasudevan

        Activity

        Hide
        hudson Hudson added a comment -

        Integrated in HBase-0.92-security #109 (See https://builds.apache.org/job/HBase-0.92-security/109/)
        HBASE-6050 HLogSplitter renaming recovered.edits and CJ removing the parent directory races, making the HBCK to think cluster is inconsistent. and a small addendum for HBASE-6002 (Ram) (Revision 1342935)
        HBASE-6002 Possible chance of resource leak in HlogSplitter (Chinna Rao) (Revision 1342932)

        Result = SUCCESS
        ramkrishna :
        Files :

        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java

        ramkrishna :
        Files :

        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        Show
        hudson Hudson added a comment - Integrated in HBase-0.92-security #109 (See https://builds.apache.org/job/HBase-0.92-security/109/ ) HBASE-6050 HLogSplitter renaming recovered.edits and CJ removing the parent directory races, making the HBCK to think cluster is inconsistent. and a small addendum for HBASE-6002 (Ram) (Revision 1342935) HBASE-6002 Possible chance of resource leak in HlogSplitter (Chinna Rao) (Revision 1342932) Result = SUCCESS ramkrishna : Files : /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java ramkrishna : Files : /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-0.94-security #33 (See https://builds.apache.org/job/HBase-0.94-security/33/)
        HBASE-6002 Possible chance of resource leak in HlogSplitter (Chinna rao) (Revision 1342931)

        Result = FAILURE
        ramkrishna :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        Show
        hudson Hudson added a comment - Integrated in HBase-0.94-security #33 (See https://builds.apache.org/job/HBase-0.94-security/33/ ) HBASE-6002 Possible chance of resource leak in HlogSplitter (Chinna rao) (Revision 1342931) Result = FAILURE ramkrishna : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #18 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/18/)
        HBASE-6002 Possible chance of resource leak in HlogSplitter (Chinna Rao) (Revision 1342929)

        Result = FAILURE
        ramkrishna :
        Files :

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #18 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/18/ ) HBASE-6002 Possible chance of resource leak in HlogSplitter (Chinna Rao) (Revision 1342929) Result = FAILURE ramkrishna : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-0.92 #424 (See https://builds.apache.org/job/HBase-0.92/424/)
        HBASE-6050 HLogSplitter renaming recovered.edits and CJ removing the parent directory races, making the HBCK to think cluster is inconsistent. and a small addendum for HBASE-6002 (Ram) (Revision 1342935)
        HBASE-6002 Possible chance of resource leak in HlogSplitter (Chinna Rao) (Revision 1342932)

        Result = FAILURE
        ramkrishna :
        Files :

        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java

        ramkrishna :
        Files :

        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        Show
        hudson Hudson added a comment - Integrated in HBase-0.92 #424 (See https://builds.apache.org/job/HBase-0.92/424/ ) HBASE-6050 HLogSplitter renaming recovered.edits and CJ removing the parent directory races, making the HBCK to think cluster is inconsistent. and a small addendum for HBASE-6002 (Ram) (Revision 1342935) HBASE-6002 Possible chance of resource leak in HlogSplitter (Chinna Rao) (Revision 1342932) Result = FAILURE ramkrishna : Files : /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java ramkrishna : Files : /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-0.94 #220 (See https://builds.apache.org/job/HBase-0.94/220/)
        HBASE-6002 Possible chance of resource leak in HlogSplitter (Chinna rao) (Revision 1342931)

        Result = FAILURE
        ramkrishna :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        Show
        hudson Hudson added a comment - Integrated in HBase-0.94 #220 (See https://builds.apache.org/job/HBase-0.94/220/ ) HBASE-6002 Possible chance of resource leak in HlogSplitter (Chinna rao) (Revision 1342931) Result = FAILURE ramkrishna : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK #2925 (See https://builds.apache.org/job/HBase-TRUNK/2925/)
        HBASE-6002 Possible chance of resource leak in HlogSplitter (Chinna Rao) (Revision 1342929)

        Result = FAILURE
        ramkrishna :
        Files :

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK #2925 (See https://builds.apache.org/job/HBase-TRUNK/2925/ ) HBASE-6002 Possible chance of resource leak in HlogSplitter (Chinna Rao) (Revision 1342929) Result = FAILURE ramkrishna : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        Hide
        ram_krish ramkrishna.s.vasudevan added a comment -

        Committed to trunk, 0.94 and 0.92.
        Thanks for the patch Chinna.
        Thanks for the review Stack and Ted.

        Show
        ram_krish ramkrishna.s.vasudevan added a comment - Committed to trunk, 0.94 and 0.92. Thanks for the patch Chinna. Thanks for the review Stack and Ted.
        Hide
        ram_krish ramkrishna.s.vasudevan added a comment -

        Patch for trunk.

        Show
        ram_krish ramkrishna.s.vasudevan added a comment - Patch for trunk.
        Hide
        ram_krish ramkrishna.s.vasudevan added a comment -

        I think the two testcases in org.apache.hadoop.hbase.regionserver.TestSplitTransactionOnCluster.testShutdownSimpleFixup and testShutdownFixupWhenDaughterHasSplit needs some clean up.

        Show
        ram_krish ramkrishna.s.vasudevan added a comment - I think the two testcases in org.apache.hadoop.hbase.regionserver.TestSplitTransactionOnCluster.testShutdownSimpleFixup and testShutdownFixupWhenDaughterHasSplit needs some clean up.
        Hide
        ram_krish ramkrishna.s.vasudevan added a comment -

        The test failures are not related to this fix. However org.apache.hadoop.hbase.regionserver.TestSplitTransactionOnCluster failure i doubted is because of HBASE-6070. But every time it passes locally . Infact the commit was done because all test cases passed locally.
        I will investigate more on that if still failure persists.

        Show
        ram_krish ramkrishna.s.vasudevan added a comment - The test failures are not related to this fix. However org.apache.hadoop.hbase.regionserver.TestSplitTransactionOnCluster failure i doubted is because of HBASE-6070 . But every time it passes locally . Infact the commit was done because all test cases passed locally. I will investigate more on that if still failure persists.
        Hide
        zhihyu@ebaysf.com Ted Yu added a comment -

        Latest patch looks Okay.
        Check out the failed tests.

        Show
        zhihyu@ebaysf.com Ted Yu added a comment - Latest patch looks Okay. Check out the failed tests.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12529760/HBASE-6002_trunk_1.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 hadoop23. The patch compiles against the hadoop 0.23.x 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 appears to introduce 33 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
        org.apache.hadoop.hbase.security.access.TestZKPermissionsWatcher
        org.apache.hadoop.hbase.master.TestSplitLogManager

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1999//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1999//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1999//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12529760/HBASE-6002_trunk_1.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 hadoop23. The patch compiles against the hadoop 0.23.x 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 appears to introduce 33 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 org.apache.hadoop.hbase.security.access.TestZKPermissionsWatcher org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1999//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1999//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1999//console This message is automatically generated.
        Hide
        ram_krish ramkrishna.s.vasudevan added a comment -

        Updated patch. I have introduced one boolean to know whether close has already been attempted or not.

        Show
        ram_krish ramkrishna.s.vasudevan added a comment - Updated patch. I have introduced one boolean to know whether close has already been attempted or not.
        Hide
        stack stack added a comment -

        Sorry. My above suggestion would do away w/ the need of the flag and it would make the flag and it would make the check more particular being done per wap.w.

        Show
        stack stack added a comment - Sorry. My above suggestion would do away w/ the need of the flag and it would make the flag and it would make the check more particular being done per wap.w.
        Hide
        stack stack added a comment -

        I'm fine w/ commit.

        Was setting wap.w = null after close discussed? (could copy it before actual call to close and then close the copy after setting wap.w = null)

        Show
        stack stack added a comment - I'm fine w/ commit. Was setting wap.w = null after close discussed? (could copy it before actual call to close and then close the copy after setting wap.w = null)
        Hide
        chinnalalam Chinna Rao Lalam added a comment -

        @Stack:
        yes writersClosed flag is used for not to attempt in finally block if the all writers are closed successfully.

        @Ted:

        "close previously closed stream has no effect"

        I think previous close successful then it wont take any effect (I am fully not sure) i need to check this.

        Show
        chinnalalam Chinna Rao Lalam added a comment - @Stack: yes writersClosed flag is used for not to attempt in finally block if the all writers are closed successfully. @Ted: "close previously closed stream has no effect" I think previous close successful then it wont take any effect (I am fully not sure) i need to check this.
        Hide
        zhihyu@ebaysf.com Ted Yu added a comment -

        The official API doc for Writer/Closeable#close only specifies that "close previously closed stream has no effect".

        FYI

        Show
        zhihyu@ebaysf.com Ted Yu added a comment - The official API doc for Writer/Closeable#close only specifies that "close previously closed stream has no effect". FYI
        Hide
        stack stack added a comment -

        Is that writersClosed flag being used? Agree we should not try and close again if first close fails. I'd be fine w/ IOE spew on fail. It should be rare. If lots of spew, probably something wrong. Can fix it later if too bad. I think avoiding the resource leak the important thing.

        Show
        stack stack added a comment - Is that writersClosed flag being used? Agree we should not try and close again if first close fails. I'd be fine w/ IOE spew on fail. It should be rare. If lots of spew, probably something wrong. Can fix it later if too bad. I think avoiding the resource leak the important thing.
        Hide
        chinnalalam Chinna Rao Lalam added a comment -

        But here my concern is close is not success let me try second time to close.
        If u feel it will generate more logging of IOE's i will update the patch. What u say..

        Show
        chinnalalam Chinna Rao Lalam added a comment - But here my concern is close is not success let me try second time to close. If u feel it will generate more logging of IOE's i will update the patch. What u say..
        Hide
        zhihyu@ebaysf.com Ted Yu added a comment -

        As you said above, introducing a closed flag wouldn't improve much.

        I was thinking about a boolean flag which marks whether close() has been attempted on the writer. This is to avoid extraneous logging of IOE's.

        Show
        zhihyu@ebaysf.com Ted Yu added a comment - As you said above, introducing a closed flag wouldn't improve much. I was thinking about a boolean flag which marks whether close() has been attempted on the writer. This is to avoid extraneous logging of IOE's.
        Hide
        chinnalalam Chinna Rao Lalam added a comment -

        @Ted:

        If add closed flag in WriterAndPath after successfull close of the writer make this flag to true. so first time if it fails flag wont be true and second time it will try to close.

        I feel same thing is achieved with the writer!=null in SequenceFileLogWriter.close(). Here after successful close of the writer it will be initialized with null. So second time if tries to close the already closed writer it will check whether it is null or not if not equal to null then only it will try to close.

         if (this.writer != null) {
              try {
                this.writer.close();
              } catch (NullPointerException npe) {
                // Can get a NPE coming up from down in DFSClient$DFSOutputStream#close
                LOG.warn(npe);
              }
              this.writer = null;
            }
        

        your comment @ 16/May/12 18:44

        If the first close encountered some IOE, calling it the second time would most likely encounter similar error.

        if the first close encountered IOE if we have closed flag in WriterAndPath it wont be make it true because writer is not succesfully closed. So second time it will try to close.

        We can use this flag as attempted or not?

        If my understanding is wrong pls correct me.

        Show
        chinnalalam Chinna Rao Lalam added a comment - @Ted: If add closed flag in WriterAndPath after successfull close of the writer make this flag to true. so first time if it fails flag wont be true and second time it will try to close. I feel same thing is achieved with the writer!=null in SequenceFileLogWriter.close(). Here after successful close of the writer it will be initialized with null. So second time if tries to close the already closed writer it will check whether it is null or not if not equal to null then only it will try to close. if ( this .writer != null ) { try { this .writer.close(); } catch (NullPointerException npe) { // Can get a NPE coming up from down in DFSClient$DFSOutputStream#close LOG.warn(npe); } this .writer = null ; } your comment @ 16/May/12 18:44 If the first close encountered some IOE, calling it the second time would most likely encounter similar error. if the first close encountered IOE if we have closed flag in WriterAndPath it wont be make it true because writer is not succesfully closed. So second time it will try to close. We can use this flag as attempted or not? If my understanding is wrong pls correct me.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12527670/HBASE-6002_trunk.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 hadoop23. The patch compiles against the hadoop 0.23.x 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 appears to introduce 31 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
        org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1894//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1894//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1894//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12527670/HBASE-6002_trunk.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 hadoop23. The patch compiles against the hadoop 0.23.x 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 appears to introduce 31 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 org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1894//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1894//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1894//console This message is automatically generated.
        Hide
        zhihyu@ebaysf.com Ted Yu added a comment -

        The same construct is used for both places when closing writer:

        +              try {
        +                wap.w.close();
        +              } catch (IOException e) {
        

        If the first close encountered some IOE, calling it the second time would most likely encounter similar error.
        My comment @ 15/May/12 21:25 applies in the above scenario.

        Show
        zhihyu@ebaysf.com Ted Yu added a comment - The same construct is used for both places when closing writer: + try { + wap.w.close(); + } catch (IOException e) { If the first close encountered some IOE, calling it the second time would most likely encounter similar error. My comment @ 15/May/12 21:25 applies in the above scenario.
        Hide
        chinnalalam Chinna Rao Lalam added a comment -

        @Ted : Updated the patch with above comment

        Show
        chinnalalam Chinna Rao Lalam added a comment - @Ted : Updated the patch with above comment
        Hide
        zhihyu@ebaysf.com Ted Yu added a comment -

        Should the following code in patch:

        +              try {
        +                wap.w.close();
        +              } catch (IOException e) {
        

        be placed around wap.w.close() call at line 475 ?

        Show
        zhihyu@ebaysf.com Ted Yu added a comment - Should the following code in patch: + try { + wap.w.close(); + } catch (IOException e) { be placed around wap.w.close() call at line 475 ?
        Hide
        chinnalalam Chinna Rao Lalam added a comment -

        wap.w.close() internally will check writer is not equal to nul or not if it is not equal to null then it will try to close. So here it wont attempt to close the already closed writer.

        Show
        chinnalalam Chinna Rao Lalam added a comment - wap.w.close() internally will check writer is not equal to nul or not if it is not equal to null then it will try to close. So here it wont attempt to close the already closed writer.
        Hide
        anoopsamjohn Anoop Sam John added a comment -

        +1 on the boolean flag maintained at each Writer level so that avoiding the close attempt again on the already closed item.

        Show
        anoopsamjohn Anoop Sam John added a comment - +1 on the boolean flag maintained at each Writer level so that avoiding the close attempt again on the already closed item.
        Hide
        zhihyu@ebaysf.com Ted Yu added a comment -
        +              try {
        +                wap.w.close();
        +              } catch (IOException e) {
        +                LOG.debug("Exception while closing the writer :", e);
        

        The above may produce considerable logs. Can boolean flag, closed, be added to WriterAndPath so that we don't try to close an already closed writer ?

        Show
        zhihyu@ebaysf.com Ted Yu added a comment - + try { + wap.w.close(); + } catch (IOException e) { + LOG.debug( "Exception while closing the writer :" , e); The above may produce considerable logs. Can boolean flag, closed, be added to WriterAndPath so that we don't try to close an already closed writer ?
        Hide
        chinnalalam Chinna Rao Lalam added a comment -

        Its good to close these writers in seperate finally block if any writer is not closed because of exception it will be closed here.

        Show
        chinnalalam Chinna Rao Lalam added a comment - Its good to close these writers in seperate finally block if any writer is not closed because of exception it will be closed here.

          People

          • Assignee:
            chinnalalam Chinna Rao Lalam
            Reporter:
            chinnalalam Chinna Rao Lalam
          • Votes:
            0 Vote for this issue
            Watchers:
            11 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development