HBase
  1. HBase
  2. HBASE-7515

Store.loadStoreFiles should close opened files if there's an exception

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.94.3
    • Fix Version/s: 0.94.5, 0.95.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Related to HBASE-7513. If a RS is able to open a few store files in Store.loadStoreFiles but one of them fails like in 7513, the opened files won't be closed and file descriptors will remain in a CLOSED_WAIT state.

      The situation we encountered is that over the weekend one region was bounced between >100 region servers and eventually they all started dying on "Too many open files".

      1. 7515-v6.txt
        4 kB
        Ted Yu
      2. 7515-v5.txt
        4 kB
        chunhui shen
      3. 7515-v4.txt
        4 kB
        Ted Yu
      4. 7515-v3.txt
        3 kB
        Ted Yu
      5. 7515-v2.txt
        2 kB
        Ted Yu
      6. 7515-0.94.txt
        4 kB
        Ted Yu
      7. 7515.txt
        2 kB
        Ted Yu

        Activity

        Hide
        Ted Yu added a comment -

        First attempt at solving this issue.

        Show
        Ted Yu added a comment - First attempt at solving this issue.
        Hide
        Elliott Clark added a comment -

        I don't think that will fix the issue. If the first store file throws the error, the rest still need to be pulled from the futures and closed.

        Show
        Elliott Clark added a comment - I don't think that will fix the issue. If the first store file throws the error, the rest still need to be pulled from the futures and closed.
        Hide
        Ted Yu added a comment -

        How about this one ?

        Show
        Ted Yu added a comment - How about this one ?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12563664/7515.txt
        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 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 lineLengths. The patch does not introduce lines longer than 100

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.TestLocalHBaseCluster
        org.apache.hadoop.hbase.io.encoding.TestUpgradeFromHFileV1ToEncoding
        org.apache.hadoop.hbase.master.TestMasterFailover

        -1 core zombie tests. There are 7 zombie test(s): at org.apache.hadoop.hbase.master.TestMasterFailover.testMasterFailoverWithMockedRITOnDeadRS(TestMasterFailover.java:833)

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//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/12563664/7515.txt 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 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 lineLengths . The patch does not introduce lines longer than 100 -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.TestLocalHBaseCluster org.apache.hadoop.hbase.io.encoding.TestUpgradeFromHFileV1ToEncoding org.apache.hadoop.hbase.master.TestMasterFailover -1 core zombie tests . There are 7 zombie test(s): at org.apache.hadoop.hbase.master.TestMasterFailover.testMasterFailoverWithMockedRITOnDeadRS(TestMasterFailover.java:833) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3920//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12563674/7515-v2.txt
        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 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 lineLengths. The patch does not introduce lines longer than 100

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.TestLocalHBaseCluster

        -1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.hdfs.server.balancer.TestBalancerWithNodeGroup.testBalancerWithRackLocality(TestBalancerWithNodeGroup.java:220)

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//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/12563674/7515-v2.txt 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 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 lineLengths . The patch does not introduce lines longer than 100 -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.TestLocalHBaseCluster -1 core zombie tests . There are 1 zombie test(s): at org.apache.hadoop.hdfs.server.balancer.TestBalancerWithNodeGroup.testBalancerWithRackLocality(TestBalancerWithNodeGroup.java:220) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3924//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        I ran TestLocalHBaseCluster locally and it passed.

        Show
        Ted Yu added a comment - I ran TestLocalHBaseCluster locally and it passed.
        Hide
        Lars Hofhansl added a comment -

        Is this only an 0.96 issue?

        Show
        Lars Hofhansl added a comment - Is this only an 0.96 issue?
        Hide
        stack added a comment -

        Lars Hofhansl No. Happened on a 0.94 cluster.

        Show
        stack added a comment - Lars Hofhansl No. Happened on a 0.94 cluster.
        Hide
        Jean-Daniel Cryans added a comment -

        How about this one ?

        The store file that threw the exception might still be opened, for example in HBASE-7513 it opens fine but the exception is thrown in another part of the code. You might also be overwriting ioe.

        Show
        Jean-Daniel Cryans added a comment - How about this one ? The store file that threw the exception might still be opened, for example in HBASE-7513 it opens fine but the exception is thrown in another part of the code. You might also be overwriting ioe .
        Hide
        Ted Yu added a comment -

        Thanks for the review, J-D.

        Patch v3 handles potential exception coming out of HStore.close().

        ioe is only created once - for the first exception caught. This way small garbage is avoided.

        Show
        Ted Yu added a comment - Thanks for the review, J-D. Patch v3 handles potential exception coming out of HStore.close(). ioe is only created once - for the first exception caught. This way small garbage is avoided.
        Hide
        Elliott Clark added a comment -

        The case where storeFile.createReader errors out is not covered by this patch. How about putting a try catch in the Callable (line 421), on exception try to close the StoreFile. Then re-throw the exception.

        Show
        Elliott Clark added a comment - The case where storeFile.createReader errors out is not covered by this patch. How about putting a try catch in the Callable (line 421), on exception try to close the StoreFile. Then re-throw the exception.
        Hide
        Ted Yu added a comment -

        I looked at storeFile.createReader call. My understanding is that the exception from the Callable would be delivered when get() is called.
        Please take a look at the example in:
        http://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/ExecutorCompletionService.html

        Show
        Ted Yu added a comment - I looked at storeFile.createReader call. My understanding is that the exception from the Callable would be delivered when get() is called. Please take a look at the example in: http://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/ExecutorCompletionService.html
        Hide
        Elliott Clark added a comment -

        How can the callable ever return something when it's thrown an exception?

        Try the example below:

        package org.apache.hadoop.hbase.regionserver;
        
        import org.junit.Test;
        
        import java.util.ArrayList;
        import java.util.List;
        import java.util.concurrent.Callable;
        import java.util.concurrent.CompletionService;
        import java.util.concurrent.ExecutorCompletionService;
        import java.util.concurrent.Executors;
        import java.util.concurrent.Future;
        
        
        public class TestFailCreate {
        
          @Test
          public void testFailCallable() throws Exception {
            CompletionService<String> completionService =
                new ExecutorCompletionService<String>(Executors.newCachedThreadPool());
        
            List<Future<String>> futureList = new ArrayList<Future<String>>();
            for (int i = 0; i < 10; i ++ ) {
              Future<String> f = completionService.submit(new Callable<String>() {
                @Override
                public String call() throws Exception {
                  String test = "STOREFILE CLOSED";
                  Thread.sleep(100);
                  test = "STORE FILE START OPEN";
                  if (true == true)  {
                    test = "STOREFILE HALF OPEN";
                    //This is simulating opening the store file. If somewhere in opening up the store
                    // file some exception is thrown.  The storefile is left in a half
                    // open state and there is no reference to it.
                    throw new Exception("TEST EXCEPTION");
                    // test = "STORE FILE FULLY OPEN"; // This line is never reached.
                  }
                  System.out.println("GOT TO THE RETURN LINE");
                  return test;
                }
              });
              futureList.add(f);
            }
        
        
            for (Future<String> f: futureList) {
              try {
                String s = f.get();
                System.out.println("Got " + s);
              } catch (Exception e) {
                System.out.println("Caught error");
              }
            }
        
        
        
          }
        }
        

        Notice how "GOT TO THE RETURN LINE" is never printed out.

        Show
        Elliott Clark added a comment - How can the callable ever return something when it's thrown an exception? Try the example below: package org.apache.hadoop.hbase.regionserver; import org.junit.Test; import java.util.ArrayList; import java.util.List; import java.util.concurrent.Callable; import java.util.concurrent.CompletionService; import java.util.concurrent.ExecutorCompletionService; import java.util.concurrent.Executors; import java.util.concurrent.Future; public class TestFailCreate { @Test public void testFailCallable() throws Exception { CompletionService< String > completionService = new ExecutorCompletionService< String >(Executors.newCachedThreadPool()); List<Future< String >> futureList = new ArrayList<Future< String >>(); for ( int i = 0; i < 10; i ++ ) { Future< String > f = completionService.submit( new Callable< String >() { @Override public String call() throws Exception { String test = "STOREFILE CLOSED" ; Thread .sleep(100); test = "STORE FILE START OPEN" ; if ( true == true ) { test = "STOREFILE HALF OPEN" ; //This is simulating opening the store file. If somewhere in opening up the store // file some exception is thrown. The storefile is left in a half // open state and there is no reference to it. throw new Exception( "TEST EXCEPTION" ); // test = "STORE FILE FULLY OPEN" ; // This line is never reached. } System .out.println( "GOT TO THE RETURN LINE" ); return test; } }); futureList.add(f); } for (Future< String > f: futureList) { try { String s = f.get(); System .out.println( "Got " + s); } catch (Exception e) { System .out.println( "Caught error" ); } } } } Notice how "GOT TO THE RETURN LINE" is never printed out.
        Hide
        Ted Yu added a comment -

        Patch v4 addresses Elliott's concern.

        Show
        Ted Yu added a comment - Patch v4 addresses Elliott's concern.
        Hide
        Lars Hofhansl added a comment -

        Man this is ugly now. But needs to be done... +1 on v4

        Show
        Lars Hofhansl added a comment - Man this is ugly now. But needs to be done... +1 on v4
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12563855/7515-v4.txt
        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 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 lineLengths. The patch does not introduce lines longer than 100

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.TestLocalHBaseCluster

        -1 core zombie tests. There are 1 zombie test(s):

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//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/12563855/7515-v4.txt 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 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 lineLengths . The patch does not introduce lines longer than 100 -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.TestLocalHBaseCluster -1 core zombie tests . There are 1 zombie test(s): Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3938//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        I ran TestLocalHBaseCluster locally and it passed:

        Running org.apache.hadoop.hbase.TestLocalHBaseCluster
        2013-01-08 16:56:16.209 java[70980:1203] Unable to load realm info from SCDynamicStore
        Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 27.618 sec
        
        Show
        Ted Yu added a comment - I ran TestLocalHBaseCluster locally and it passed: Running org.apache.hadoop.hbase.TestLocalHBaseCluster 2013-01-08 16:56:16.209 java[70980:1203] Unable to load realm info from SCDynamicStore Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 27.618 sec
        Hide
        Ted Yu added a comment -

        Jean-Daniel Cryans, Elliott Clark:
        Is there any place that I missed ?

        Thanks

        Show
        Ted Yu added a comment - Jean-Daniel Cryans , Elliott Clark : Is there any place that I missed ? Thanks
        Hide
        chunhui shen added a comment -

        I think patch v4 is ok for the correctness.

        How about using finally like the following?

        
        try {
                    storeFile.createReader();
                  } finally{
                    return storeFile;
                  }
                   
        
        Show
        chunhui shen added a comment - I think patch v4 is ok for the correctness. How about using finally like the following? try { storeFile.createReader(); } finally { return storeFile; }
        Hide
        chunhui shen added a comment -

        Sorry, the above comment is wrong.

        Patch-v5 move the location of adding storefile to results

        Just for a reference

        Show
        chunhui shen added a comment - Sorry, the above comment is wrong. Patch-v5 move the location of adding storefile to results Just for a reference
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12563908/7515-v5.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 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3945//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/12563908/7515-v5.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 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3945//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12563910/7515-v5.txt
        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 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        -1 javadoc. The javadoc tool appears to have generated 1 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 lineLengths. The patch does not introduce lines longer than 100

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.TestLocalHBaseCluster

        -1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.hdfs.server.balancer.TestBalancerWithNodeGroup.testBalancerWithRackLocality(TestBalancerWithNodeGroup.java:220)

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//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/12563910/7515-v5.txt 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 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 1 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 lineLengths . The patch does not introduce lines longer than 100 -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.TestLocalHBaseCluster -1 core zombie tests . There are 1 zombie test(s): at org.apache.hadoop.hdfs.server.balancer.TestBalancerWithNodeGroup.testBalancerWithRackLocality(TestBalancerWithNodeGroup.java:220) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3946//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        +1 on V5.
        @Chunhui
        What specific difference we will get between v4 and v5?
        Anyway in v4 do we try to close the reader if open fails? May be we can just throw exception alone.

        Show
        ramkrishna.s.vasudevan added a comment - +1 on V5. @Chunhui What specific difference we will get between v4 and v5? Anyway in v4 do we try to close the reader if open fails? May be we can just throw exception alone.
        Hide
        Ted Yu added a comment -

        in v4 do we try to close the reader if open fails?

        Yes:

        +            storeFile.createReader();
        +          } catch (IOException e) {
        +            try {
        +              storeFile.closeReader(true);
        
        Show
        Ted Yu added a comment - in v4 do we try to close the reader if open fails? Yes: + storeFile.createReader(); + } catch (IOException e) { + try { + storeFile.closeReader( true );
        Hide
        Elliott Clark added a comment -

        v4 looks better to me than v5. I don't really like adding synchronized bits just for cosmetic code reasons.

        However JD had a good idea. Closing the reader that throws an exception could be pushed up into to storefile.createReader(). Then what happens is that any store file that errors out has already cleaned up after itself. Then we just have to close the other store files that didn't error out. That makes things a little cleaner in my opinion.

        Show
        Elliott Clark added a comment - v4 looks better to me than v5. I don't really like adding synchronized bits just for cosmetic code reasons. However JD had a good idea. Closing the reader that throws an exception could be pushed up into to storefile.createReader(). Then what happens is that any store file that errors out has already cleaned up after itself. Then we just have to close the other store files that didn't error out. That makes things a little cleaner in my opinion.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Anyway in v4 do we try to close the reader if open fails?

        I got now after seeing the storeFile.open() code. There is a chance that the reader has been partially initialized and so closing it makes sense.

        Show
        ramkrishna.s.vasudevan added a comment - Anyway in v4 do we try to close the reader if open fails? I got now after seeing the storeFile.open() code. There is a chance that the reader has been partially initialized and so closing it makes sense.
        Hide
        Ted Yu added a comment -

        Patch v6 adds exception handling in createReader()

        Show
        Ted Yu added a comment - Patch v6 adds exception handling in createReader()
        Hide
        Elliott Clark added a comment -

        +1 v6

        Show
        Elliott Clark added a comment - +1 v6
        Hide
        Lars Hofhansl added a comment -

        v6 looks good to me +1.
        Is somebody volunteering a 0.94 patch? If not, I'll create one.

        Show
        Lars Hofhansl added a comment - v6 looks good to me +1. Is somebody volunteering a 0.94 patch? If not, I'll create one.
        Hide
        Ted Yu added a comment -

        Patch for 0.94

        Show
        Ted Yu added a comment - Patch for 0.94
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12563976/7515-v6.txt
        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 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        -1 javadoc. The javadoc tool appears to have generated 1 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 lineLengths. The patch does not introduce lines longer than 100

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.TestLocalHBaseCluster
        org.apache.hadoop.hbase.replication.TestReplication

        -1 core zombie tests. There are 2 zombie test(s):

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//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/12563976/7515-v6.txt 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 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 1 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 lineLengths . The patch does not introduce lines longer than 100 -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.TestLocalHBaseCluster org.apache.hadoop.hbase.replication.TestReplication -1 core zombie tests . There are 2 zombie test(s): Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3950//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        Integrated to trunk and 0.94

        Thanks for the review, Lars, J-D, Elliot and Chunhui.

        Show
        Ted Yu added a comment - Integrated to trunk and 0.94 Thanks for the review, Lars, J-D, Elliot and Chunhui.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #3716 (See https://builds.apache.org/job/HBase-TRUNK/3716/)
        HBASE-7515 Store.loadStoreFiles should close opened files if there's an exception (Ted Yu) (Revision 1431045)

        Result = FAILURE
        tedyu :
        Files :

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #3716 (See https://builds.apache.org/job/HBase-TRUNK/3716/ ) HBASE-7515 Store.loadStoreFiles should close opened files if there's an exception (Ted Yu) (Revision 1431045) Result = FAILURE tedyu : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #719 (See https://builds.apache.org/job/HBase-0.94/719/)
        HBASE-7515 Store.loadStoreFiles should close opened files if there's an exception (Ted Yu) (Revision 1431046)

        Result = FAILURE
        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #719 (See https://builds.apache.org/job/HBase-0.94/719/ ) HBASE-7515 Store.loadStoreFiles should close opened files if there's an exception (Ted Yu) (Revision 1431046) Result = FAILURE tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #338 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/338/)
        HBASE-7515 Store.loadStoreFiles should close opened files if there's an exception (Ted Yu) (Revision 1431045)

        Result = FAILURE
        tedyu :
        Files :

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #338 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/338/ ) HBASE-7515 Store.loadStoreFiles should close opened files if there's an exception (Ted Yu) (Revision 1431045) Result = FAILURE tedyu : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security #95 (See https://builds.apache.org/job/HBase-0.94-security/95/)
        HBASE-7515 Store.loadStoreFiles should close opened files if there's an exception (Ted Yu) (Revision 1431046)

        Result = SUCCESS
        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security #95 (See https://builds.apache.org/job/HBase-0.94-security/95/ ) HBASE-7515 Store.loadStoreFiles should close opened files if there's an exception (Ted Yu) (Revision 1431046) Result = SUCCESS tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security-on-Hadoop-23 #11 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/11/)
        HBASE-7515 Store.loadStoreFiles should close opened files if there's an exception (Ted Yu) (Revision 1431046)

        Result = FAILURE
        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security-on-Hadoop-23 #11 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/11/ ) HBASE-7515 Store.loadStoreFiles should close opened files if there's an exception (Ted Yu) (Revision 1431046) Result = FAILURE tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java

          People

          • Assignee:
            Ted Yu
            Reporter:
            Jean-Daniel Cryans
          • Votes:
            0 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development