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

HStoreFile should be closed in HStore#hasReferences

Details

    • Reviewed

    Description

            reloadedStoreFiles = loadStoreFiles();
            return StoreUtils.hasReferences(reloadedStoreFiles);
      

      The intention of obtaining the HStoreFile's is to check for references.
      The loaded HStoreFile's should be closed prior to return to prevent leak.

      I noticed the increase in open files when running test suite. After checking recently modified code, I came to this particular method.

      Attachments

        1. 21088.v1.txt
          1 kB
          Ted Yu
        2. 21088.v2.txt
          1 kB
          Ted Yu
        3. 21088.v2.txt
          1 kB
          Ted Yu
        4. 21088.v3.txt
          0.9 kB
          Ted Yu
        5. 21088.v4.txt
          1.0 kB
          Ted Yu

        Activity

          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 11s Docker mode activated.
          0 patch 0m 1s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.7.0/precommit-patchnames for instructions.
                Prechecks
          +1 hbaseanti 0m 0s Patch does not have any anti-patterns.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -0 test4tests 0m 0s 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.
                master Compile Tests
          +1 mvninstall 4m 12s master passed
          +1 compile 2m 8s master passed
          +1 checkstyle 1m 22s master passed
          +1 shadedjars 5m 2s branch has no errors when building our shaded downstream artifacts.
          +1 findbugs 2m 22s master passed
          +1 javadoc 0m 35s master passed
                Patch Compile Tests
          +1 mvninstall 4m 14s the patch passed
          +1 compile 2m 0s the patch passed
          +1 javac 2m 0s the patch passed
          +1 checkstyle 1m 19s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 shadedjars 4m 51s patch has no errors when building our shaded downstream artifacts.
          +1 hadoopcheck 9m 12s Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0.
          +1 findbugs 2m 41s the patch passed
          +1 javadoc 0m 37s the patch passed
                Other Tests
          -1 unit 128m 25s hbase-server in the patch failed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          170m 9s



          Reason Tests
          Failed junit tests hadoop.hbase.replication.regionserver.TestSyncReplicationShipperQuit



          Subsystem Report/Notes
          Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:b002b0b
          JIRA Issue HBASE-21088
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12936476/21088.v1.txt
          Optional Tests asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
          uname Linux 69423d60acb3 3.13.0-143-generic #192-Ubuntu SMP Tue Feb 27 10:45:36 UTC 2018 x86_64 GNU/Linux
          Build tool maven
          Personality /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build@2/component/dev-support/hbase-personality.sh
          git revision master / 588ff921c1
          maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z)
          Default Java 1.8.0_181
          findbugs v3.1.0-RC3
          unit https://builds.apache.org/job/PreCommit-HBASE-Build/14118/artifact/patchprocess/patch-unit-hbase-server.txt
          Test Results https://builds.apache.org/job/PreCommit-HBASE-Build/14118/testReport/
          Max. process+thread count 5097 (vs. ulimit of 10000)
          modules C: hbase-server U: hbase-server
          Console output https://builds.apache.org/job/PreCommit-HBASE-Build/14118/console
          Powered by Apache Yetus 0.7.0 http://yetus.apache.org

          This message was automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 11s Docker mode activated. 0 patch 0m 1s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.7.0/precommit-patchnames for instructions.       Prechecks +1 hbaseanti 0m 0s Patch does not have any anti-patterns. +1 @author 0m 0s The patch does not contain any @author tags. -0 test4tests 0m 0s 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.       master Compile Tests +1 mvninstall 4m 12s master passed +1 compile 2m 8s master passed +1 checkstyle 1m 22s master passed +1 shadedjars 5m 2s branch has no errors when building our shaded downstream artifacts. +1 findbugs 2m 22s master passed +1 javadoc 0m 35s master passed       Patch Compile Tests +1 mvninstall 4m 14s the patch passed +1 compile 2m 0s the patch passed +1 javac 2m 0s the patch passed +1 checkstyle 1m 19s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedjars 4m 51s patch has no errors when building our shaded downstream artifacts. +1 hadoopcheck 9m 12s Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0. +1 findbugs 2m 41s the patch passed +1 javadoc 0m 37s the patch passed       Other Tests -1 unit 128m 25s hbase-server in the patch failed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 170m 9s Reason Tests Failed junit tests hadoop.hbase.replication.regionserver.TestSyncReplicationShipperQuit Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:b002b0b JIRA Issue HBASE-21088 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12936476/21088.v1.txt Optional Tests asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile uname Linux 69423d60acb3 3.13.0-143-generic #192-Ubuntu SMP Tue Feb 27 10:45:36 UTC 2018 x86_64 GNU/Linux Build tool maven Personality /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build@2/component/dev-support/hbase-personality.sh git revision master / 588ff921c1 maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z) Default Java 1.8.0_181 findbugs v3.1.0-RC3 unit https://builds.apache.org/job/PreCommit-HBASE-Build/14118/artifact/patchprocess/patch-unit-hbase-server.txt Test Results https://builds.apache.org/job/PreCommit-HBASE-Build/14118/testReport/ Max. process+thread count 5097 (vs. ulimit of 10000) modules C: hbase-server U: hbase-server Console output https://builds.apache.org/job/PreCommit-HBASE-Build/14118/console Powered by Apache Yetus 0.7.0 http://yetus.apache.org This message was automatically generated.
          stack Michael Stack added a comment -

          Where did this issue come from? A bug found in a deploy? A debug session? Or is it just random code reading? Without it, there is no sense of import, or damage done, or rate of incidence?

          On this issue, why not a finally block? The IOE could come out and the files will be still open, no?

          stack Michael Stack added a comment - Where did this issue come from? A bug found in a deploy? A debug session? Or is it just random code reading? Without it, there is no sense of import, or damage done, or rate of incidence? On this issue, why not a finally block? The IOE could come out and the files will be still open, no?
          yuzhihong@gmail.com Ted Yu added a comment -

          Where did this issue come from? A bug found in a deploy? A debug session? Or is it just random code reading?

          I noticed the increase in open files when running test suite. After checking recently modified code, I came to this particular method.

          Without it, there is no sense of import, or damage done, or rate of incidence?

          The implication of current code is that many open StoreFile's would be dangling after hasReferences() returns.

          why not a finally block?

          The finally block would be applied to StoreUtils.hasReferences.
          However, HStoreFile::isReference is used to check for reference which isn't declared to throw IOE.
          If you still think finally block is needed, I can add it in the next patch.

          The IOE could come out and the files will be still open, no?

          Looking at the following method:

            private List<HStoreFile> openStoreFiles(Collection<StoreFileInfo> files) throws IOException {
          

          we can see that open StoreFile readers would be closed in case of IOE:

              if (ioe != null) {
                // close StoreFile readers
                boolean evictOnClose =
                    cacheConf != null? cacheConf.shouldEvictOnClose(): true;
                for (HStoreFile file : results) {
          

          So there is no more open reader to close from the point of view of hasReferences().

          yuzhihong@gmail.com Ted Yu added a comment - Where did this issue come from? A bug found in a deploy? A debug session? Or is it just random code reading? I noticed the increase in open files when running test suite. After checking recently modified code, I came to this particular method. Without it, there is no sense of import, or damage done, or rate of incidence? The implication of current code is that many open StoreFile's would be dangling after hasReferences() returns. why not a finally block? The finally block would be applied to StoreUtils.hasReferences . However, HStoreFile::isReference is used to check for reference which isn't declared to throw IOE. If you still think finally block is needed, I can add it in the next patch. The IOE could come out and the files will be still open, no? Looking at the following method: private List<HStoreFile> openStoreFiles(Collection<StoreFileInfo> files) throws IOException { we can see that open StoreFile readers would be closed in case of IOE: if (ioe != null ) { // close StoreFile readers boolean evictOnClose = cacheConf != null ? cacheConf.shouldEvictOnClose(): true ; for (HStoreFile file : results) { So there is no more open reader to close from the point of view of hasReferences().
          stack Michael Stack added a comment -

          I noticed the increase in open files when running test suite.

          Next time, this would be a good lead-off when creating an issue... Its a fine objective... How you arrived at the bug and what you are trying to address. The curt description at the head of this issue says nothing. It makes you look like you are erratic in where you apply your attention.

          Your reasoning on whether or not to close on IOE is probably good but doesn't it strike you that what is going on here is hard-to-figure and cryptic, likely to break if mods made in subroutines? Adding the finally even if it might end up in double-close seems like safe route to me.

          stack Michael Stack added a comment - I noticed the increase in open files when running test suite. Next time, this would be a good lead-off when creating an issue... Its a fine objective... How you arrived at the bug and what you are trying to address. The curt description at the head of this issue says nothing. It makes you look like you are erratic in where you apply your attention. Your reasoning on whether or not to close on IOE is probably good but doesn't it strike you that what is going on here is hard-to-figure and cryptic, likely to break if mods made in subroutines? Adding the finally even if it might end up in double-close seems like safe route to me.
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 19s Docker mode activated.
          0 patch 0m 2s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.7.0/precommit-patchnames for instructions.
                Prechecks
          +1 hbaseanti 0m 0s Patch does not have any anti-patterns.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -0 test4tests 0m 0s 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.
                master Compile Tests
          +1 mvninstall 6m 58s master passed
          +1 compile 1m 51s master passed
          +1 checkstyle 1m 7s master passed
          +1 shadedjars 4m 13s branch has no errors when building our shaded downstream artifacts.
          +1 findbugs 2m 3s master passed
          +1 javadoc 0m 40s master passed
                Patch Compile Tests
          +1 mvninstall 3m 50s the patch passed
          +1 compile 1m 51s the patch passed
          +1 javac 1m 51s the patch passed
          +1 checkstyle 1m 4s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 shadedjars 4m 2s patch has no errors when building our shaded downstream artifacts.
          +1 hadoopcheck 7m 28s Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0.
          +1 findbugs 2m 14s the patch passed
          +1 javadoc 0m 30s the patch passed
                Other Tests
          -1 unit 224m 22s hbase-server in the patch failed.
          +1 asflicense 0m 27s The patch does not generate ASF License warnings.
          263m 33s



          Reason Tests
          Failed junit tests hadoop.hbase.master.procedure.TestEnableTableProcedure
            hadoop.hbase.client.TestBlockEvictionFromClient
            hadoop.hbase.quotas.TestSpaceQuotas
            hadoop.hbase.client.TestAsyncRegionAdminApi
            hadoop.hbase.master.assignment.TestMergeTableRegionsProcedure



          Subsystem Report/Notes
          Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:b002b0b
          JIRA Issue HBASE-21088
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12936520/21088.v2.txt
          Optional Tests asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
          uname Linux 8079fe522772 4.4.0-130-generic #156-Ubuntu SMP Thu Jun 14 08:53:28 UTC 2018 x86_64 GNU/Linux
          Build tool maven
          Personality /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh
          git revision master / f62c8201b6
          maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z)
          Default Java 1.8.0_181
          findbugs v3.1.0-RC3
          unit https://builds.apache.org/job/PreCommit-HBASE-Build/14122/artifact/patchprocess/patch-unit-hbase-server.txt
          Test Results https://builds.apache.org/job/PreCommit-HBASE-Build/14122/testReport/
          Max. process+thread count 4654 (vs. ulimit of 10000)
          modules C: hbase-server U: hbase-server
          Console output https://builds.apache.org/job/PreCommit-HBASE-Build/14122/console
          Powered by Apache Yetus 0.7.0 http://yetus.apache.org

          This message was automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 19s Docker mode activated. 0 patch 0m 2s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.7.0/precommit-patchnames for instructions.       Prechecks +1 hbaseanti 0m 0s Patch does not have any anti-patterns. +1 @author 0m 0s The patch does not contain any @author tags. -0 test4tests 0m 0s 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.       master Compile Tests +1 mvninstall 6m 58s master passed +1 compile 1m 51s master passed +1 checkstyle 1m 7s master passed +1 shadedjars 4m 13s branch has no errors when building our shaded downstream artifacts. +1 findbugs 2m 3s master passed +1 javadoc 0m 40s master passed       Patch Compile Tests +1 mvninstall 3m 50s the patch passed +1 compile 1m 51s the patch passed +1 javac 1m 51s the patch passed +1 checkstyle 1m 4s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedjars 4m 2s patch has no errors when building our shaded downstream artifacts. +1 hadoopcheck 7m 28s Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0. +1 findbugs 2m 14s the patch passed +1 javadoc 0m 30s the patch passed       Other Tests -1 unit 224m 22s hbase-server in the patch failed. +1 asflicense 0m 27s The patch does not generate ASF License warnings. 263m 33s Reason Tests Failed junit tests hadoop.hbase.master.procedure.TestEnableTableProcedure   hadoop.hbase.client.TestBlockEvictionFromClient   hadoop.hbase.quotas.TestSpaceQuotas   hadoop.hbase.client.TestAsyncRegionAdminApi   hadoop.hbase.master.assignment.TestMergeTableRegionsProcedure Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:b002b0b JIRA Issue HBASE-21088 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12936520/21088.v2.txt Optional Tests asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile uname Linux 8079fe522772 4.4.0-130-generic #156-Ubuntu SMP Thu Jun 14 08:53:28 UTC 2018 x86_64 GNU/Linux Build tool maven Personality /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh git revision master / f62c8201b6 maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z) Default Java 1.8.0_181 findbugs v3.1.0-RC3 unit https://builds.apache.org/job/PreCommit-HBASE-Build/14122/artifact/patchprocess/patch-unit-hbase-server.txt Test Results https://builds.apache.org/job/PreCommit-HBASE-Build/14122/testReport/ Max. process+thread count 4654 (vs. ulimit of 10000) modules C: hbase-server U: hbase-server Console output https://builds.apache.org/job/PreCommit-HBASE-Build/14122/console Powered by Apache Yetus 0.7.0 http://yetus.apache.org This message was automatically generated.
          yuzhihong@gmail.com Ted Yu added a comment -

          The procedure test failures were not related to the patch.

          I ran TestAsyncRegionAdminApi, TestBlockEvictionFromClient and TestSpaceQuotas locally which passed.

          yuzhihong@gmail.com Ted Yu added a comment - The procedure test failures were not related to the patch. I ran TestAsyncRegionAdminApi, TestBlockEvictionFromClient and TestSpaceQuotas locally which passed.
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 9s Docker mode activated.
          0 patch 0m 1s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.7.0/precommit-patchnames for instructions.
                Prechecks
          +1 hbaseanti 0m 0s Patch does not have any anti-patterns.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -0 test4tests 0m 0s 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.
                master Compile Tests
          +1 mvninstall 4m 11s master passed
          +1 compile 1m 54s master passed
          +1 checkstyle 1m 19s master passed
          +1 shadedjars 4m 45s branch has no errors when building our shaded downstream artifacts.
          +1 findbugs 2m 20s master passed
          +1 javadoc 0m 32s master passed
                Patch Compile Tests
          +1 mvninstall 3m 42s the patch passed
          +1 compile 1m 55s the patch passed
          +1 javac 1m 55s the patch passed
          +1 checkstyle 1m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 shadedjars 4m 13s patch has no errors when building our shaded downstream artifacts.
          +1 hadoopcheck 8m 2s Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0.
          +1 findbugs 2m 11s the patch passed
          +1 javadoc 0m 31s the patch passed
                Other Tests
          +1 unit 120m 22s hbase-server in the patch passed.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          158m 10s



          Subsystem Report/Notes
          Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:b002b0b
          JIRA Issue HBASE-21088
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12936553/21088.v2.txt
          Optional Tests asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
          uname Linux a30f626c190b 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9 14:43:09 UTC 2018 x86_64 GNU/Linux
          Build tool maven
          Personality /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh
          git revision master / 50055dbf04
          maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z)
          Default Java 1.8.0_181
          findbugs v3.1.0-RC3
          Test Results https://builds.apache.org/job/PreCommit-HBASE-Build/14125/testReport/
          Max. process+thread count 4486 (vs. ulimit of 10000)
          modules C: hbase-server U: hbase-server
          Console output https://builds.apache.org/job/PreCommit-HBASE-Build/14125/console
          Powered by Apache Yetus 0.7.0 http://yetus.apache.org

          This message was automatically generated.

          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 9s Docker mode activated. 0 patch 0m 1s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.7.0/precommit-patchnames for instructions.       Prechecks +1 hbaseanti 0m 0s Patch does not have any anti-patterns. +1 @author 0m 0s The patch does not contain any @author tags. -0 test4tests 0m 0s 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.       master Compile Tests +1 mvninstall 4m 11s master passed +1 compile 1m 54s master passed +1 checkstyle 1m 19s master passed +1 shadedjars 4m 45s branch has no errors when building our shaded downstream artifacts. +1 findbugs 2m 20s master passed +1 javadoc 0m 32s master passed       Patch Compile Tests +1 mvninstall 3m 42s the patch passed +1 compile 1m 55s the patch passed +1 javac 1m 55s the patch passed +1 checkstyle 1m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedjars 4m 13s patch has no errors when building our shaded downstream artifacts. +1 hadoopcheck 8m 2s Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0. +1 findbugs 2m 11s the patch passed +1 javadoc 0m 31s the patch passed       Other Tests +1 unit 120m 22s hbase-server in the patch passed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 158m 10s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:b002b0b JIRA Issue HBASE-21088 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12936553/21088.v2.txt Optional Tests asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile uname Linux a30f626c190b 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9 14:43:09 UTC 2018 x86_64 GNU/Linux Build tool maven Personality /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh git revision master / 50055dbf04 maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z) Default Java 1.8.0_181 findbugs v3.1.0-RC3 Test Results https://builds.apache.org/job/PreCommit-HBASE-Build/14125/testReport/ Max. process+thread count 4486 (vs. ulimit of 10000) modules C: hbase-server U: hbase-server Console output https://builds.apache.org/job/PreCommit-HBASE-Build/14125/console Powered by Apache Yetus 0.7.0 http://yetus.apache.org This message was automatically generated.
          stack Michael Stack added a comment -

          Ain't it loadStoreFiles that is opening the files? If so, your try/finally is in the wrong place?

          stack Michael Stack added a comment - Ain't it loadStoreFiles that is opening the files? If so, your try/finally is in the wrong place?
          yuzhihong@gmail.com Ted Yu added a comment -

          As I explained at the end of this comment:

          https://issues.apache.org/jira/browse/HBASE-21088?focusedCommentId=16587986&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16587986

          If loadStoreFiles() encounters exception, it would close already opened store file readers.
          When control returns to hasReferences(), reloadedStoreFiles would not be assigned.
          There is nothing much that can be done in hasReferences() under this scenario.

          If StoreUtils.hasReferences() throws any exception (possibly due to code change in the future), the opened store files would be closed (in patch v2).

          yuzhihong@gmail.com Ted Yu added a comment - As I explained at the end of this comment: https://issues.apache.org/jira/browse/HBASE-21088?focusedCommentId=16587986&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16587986 If loadStoreFiles() encounters exception, it would close already opened store file readers. When control returns to hasReferences(), reloadedStoreFiles would not be assigned. There is nothing much that can be done in hasReferences() under this scenario. If StoreUtils.hasReferences() throws any exception (possibly due to code change in the future), the opened store files would be closed (in patch v2).
          stack Michael Stack added a comment -

          Yeah. Seems totally cryptic to me.

          Suggest something like this where the close is connected to the location at which the files are opened:

          try {
            reloadedStoreFiles = loadStoreFiles();
            return StoreUtils.hasReferences(reloadedStoreFiles);
          } finally {
            ..... do close
          }
          
          stack Michael Stack added a comment - Yeah. Seems totally cryptic to me. Suggest something like this where the close is connected to the location at which the files are opened: try { reloadedStoreFiles = loadStoreFiles(); return StoreUtils.hasReferences(reloadedStoreFiles); } finally { ..... do close }
          stack Michael Stack added a comment -

          That makes more sense to me at least (perhaps it just me). Patch is smaller too.

          I think you should log a WARN here:

          // continue with closing the remaining store files

          ... rather than have an empty catch block. Can say you are going to continue... But if problem closing files, we probably want to know about it. Thanks.

          stack Michael Stack added a comment - That makes more sense to me at least (perhaps it just me). Patch is smaller too. I think you should log a WARN here: // continue with closing the remaining store files ... rather than have an empty catch block. Can say you are going to continue... But if problem closing files, we probably want to know about it. Thanks.
          stack Michael Stack added a comment -

          LGTM

          stack Michael Stack added a comment - LGTM
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s Docker mode activated.
          0 patch 0m 1s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.7.0/precommit-patchnames for instructions.
                Prechecks
          +1 hbaseanti 0m 0s Patch does not have any anti-patterns.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -0 test4tests 0m 0s 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.
                master Compile Tests
          +1 mvninstall 5m 6s master passed
          +1 compile 2m 14s master passed
          +1 checkstyle 1m 28s master passed
          +1 shadedjars 4m 55s branch has no errors when building our shaded downstream artifacts.
          +1 findbugs 2m 28s master passed
          +1 javadoc 0m 36s master passed
                Patch Compile Tests
          +1 mvninstall 4m 14s the patch passed
          +1 compile 2m 5s the patch passed
          +1 javac 2m 5s the patch passed
          +1 checkstyle 1m 21s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 shadedjars 4m 41s patch has no errors when building our shaded downstream artifacts.
          +1 hadoopcheck 7m 54s Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0.
          +1 findbugs 2m 10s the patch passed
          +1 javadoc 0m 29s the patch passed
                Other Tests
          -1 unit 118m 47s hbase-server in the patch failed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          159m 37s



          Reason Tests
          Failed junit tests hadoop.hbase.regionserver.throttle.TestFlushWithThroughputController



          Subsystem Report/Notes
          Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:b002b0b
          JIRA Issue HBASE-21088
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12936673/21088.v3.txt
          Optional Tests asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
          uname Linux edf6c260ca7b 3.13.0-143-generic #192-Ubuntu SMP Tue Feb 27 10:45:36 UTC 2018 x86_64 GNU/Linux
          Build tool maven
          Personality /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build@2/component/dev-support/hbase-personality.sh
          git revision master / 77a6bf3b33
          maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z)
          Default Java 1.8.0_181
          findbugs v3.1.0-RC3
          unit https://builds.apache.org/job/PreCommit-HBASE-Build/14137/artifact/patchprocess/patch-unit-hbase-server.txt
          Test Results https://builds.apache.org/job/PreCommit-HBASE-Build/14137/testReport/
          Max. process+thread count 4539 (vs. ulimit of 10000)
          modules C: hbase-server U: hbase-server
          Console output https://builds.apache.org/job/PreCommit-HBASE-Build/14137/console
          Powered by Apache Yetus 0.7.0 http://yetus.apache.org

          This message was automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s Docker mode activated. 0 patch 0m 1s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.7.0/precommit-patchnames for instructions.       Prechecks +1 hbaseanti 0m 0s Patch does not have any anti-patterns. +1 @author 0m 0s The patch does not contain any @author tags. -0 test4tests 0m 0s 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.       master Compile Tests +1 mvninstall 5m 6s master passed +1 compile 2m 14s master passed +1 checkstyle 1m 28s master passed +1 shadedjars 4m 55s branch has no errors when building our shaded downstream artifacts. +1 findbugs 2m 28s master passed +1 javadoc 0m 36s master passed       Patch Compile Tests +1 mvninstall 4m 14s the patch passed +1 compile 2m 5s the patch passed +1 javac 2m 5s the patch passed +1 checkstyle 1m 21s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedjars 4m 41s patch has no errors when building our shaded downstream artifacts. +1 hadoopcheck 7m 54s Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0. +1 findbugs 2m 10s the patch passed +1 javadoc 0m 29s the patch passed       Other Tests -1 unit 118m 47s hbase-server in the patch failed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 159m 37s Reason Tests Failed junit tests hadoop.hbase.regionserver.throttle.TestFlushWithThroughputController Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:b002b0b JIRA Issue HBASE-21088 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12936673/21088.v3.txt Optional Tests asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile uname Linux edf6c260ca7b 3.13.0-143-generic #192-Ubuntu SMP Tue Feb 27 10:45:36 UTC 2018 x86_64 GNU/Linux Build tool maven Personality /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build@2/component/dev-support/hbase-personality.sh git revision master / 77a6bf3b33 maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z) Default Java 1.8.0_181 findbugs v3.1.0-RC3 unit https://builds.apache.org/job/PreCommit-HBASE-Build/14137/artifact/patchprocess/patch-unit-hbase-server.txt Test Results https://builds.apache.org/job/PreCommit-HBASE-Build/14137/testReport/ Max. process+thread count 4539 (vs. ulimit of 10000) modules C: hbase-server U: hbase-server Console output https://builds.apache.org/job/PreCommit-HBASE-Build/14137/console Powered by Apache Yetus 0.7.0 http://yetus.apache.org This message was automatically generated.
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 11s Docker mode activated.
          0 patch 0m 2s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.7.0/precommit-patchnames for instructions.
                Prechecks
          +1 hbaseanti 0m 0s Patch does not have any anti-patterns.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -0 test4tests 0m 0s 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.
                master Compile Tests
          +1 mvninstall 3m 43s master passed
          +1 compile 1m 47s master passed
          +1 checkstyle 1m 12s master passed
          +1 shadedjars 4m 16s branch has no errors when building our shaded downstream artifacts.
          +1 findbugs 2m 8s master passed
          +1 javadoc 0m 31s master passed
                Patch Compile Tests
          +1 mvninstall 3m 44s the patch passed
          +1 compile 1m 50s the patch passed
          +1 javac 1m 50s the patch passed
          +1 checkstyle 1m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 shadedjars 4m 24s patch has no errors when building our shaded downstream artifacts.
          +1 hadoopcheck 7m 27s Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0.
          +1 findbugs 2m 1s the patch passed
          +1 javadoc 0m 30s the patch passed
                Other Tests
          -1 unit 121m 15s hbase-server in the patch failed.
          +1 asflicense 0m 23s The patch does not generate ASF License warnings.
          157m 13s



          Reason Tests
          Failed junit tests hadoop.hbase.regionserver.throttle.TestFlushWithThroughputController



          Subsystem Report/Notes
          Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:b002b0b
          JIRA Issue HBASE-21088
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12936688/21088.v4.txt
          Optional Tests asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
          uname Linux 785fb3a77922 3.13.0-143-generic #192-Ubuntu SMP Tue Feb 27 10:45:36 UTC 2018 x86_64 GNU/Linux
          Build tool maven
          Personality /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh
          git revision master / b0af08bf9b
          maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z)
          Default Java 1.8.0_181
          findbugs v3.1.0-RC3
          unit https://builds.apache.org/job/PreCommit-HBASE-Build/14138/artifact/patchprocess/patch-unit-hbase-server.txt
          Test Results https://builds.apache.org/job/PreCommit-HBASE-Build/14138/testReport/
          Max. process+thread count 4687 (vs. ulimit of 10000)
          modules C: hbase-server U: hbase-server
          Console output https://builds.apache.org/job/PreCommit-HBASE-Build/14138/console
          Powered by Apache Yetus 0.7.0 http://yetus.apache.org

          This message was automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 11s Docker mode activated. 0 patch 0m 2s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.7.0/precommit-patchnames for instructions.       Prechecks +1 hbaseanti 0m 0s Patch does not have any anti-patterns. +1 @author 0m 0s The patch does not contain any @author tags. -0 test4tests 0m 0s 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.       master Compile Tests +1 mvninstall 3m 43s master passed +1 compile 1m 47s master passed +1 checkstyle 1m 12s master passed +1 shadedjars 4m 16s branch has no errors when building our shaded downstream artifacts. +1 findbugs 2m 8s master passed +1 javadoc 0m 31s master passed       Patch Compile Tests +1 mvninstall 3m 44s the patch passed +1 compile 1m 50s the patch passed +1 javac 1m 50s the patch passed +1 checkstyle 1m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedjars 4m 24s patch has no errors when building our shaded downstream artifacts. +1 hadoopcheck 7m 27s Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0. +1 findbugs 2m 1s the patch passed +1 javadoc 0m 30s the patch passed       Other Tests -1 unit 121m 15s hbase-server in the patch failed. +1 asflicense 0m 23s The patch does not generate ASF License warnings. 157m 13s Reason Tests Failed junit tests hadoop.hbase.regionserver.throttle.TestFlushWithThroughputController Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hbase:b002b0b JIRA Issue HBASE-21088 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12936688/21088.v4.txt Optional Tests asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile uname Linux 785fb3a77922 3.13.0-143-generic #192-Ubuntu SMP Tue Feb 27 10:45:36 UTC 2018 x86_64 GNU/Linux Build tool maven Personality /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh git revision master / b0af08bf9b maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z) Default Java 1.8.0_181 findbugs v3.1.0-RC3 unit https://builds.apache.org/job/PreCommit-HBASE-Build/14138/artifact/patchprocess/patch-unit-hbase-server.txt Test Results https://builds.apache.org/job/PreCommit-HBASE-Build/14138/testReport/ Max. process+thread count 4687 (vs. ulimit of 10000) modules C: hbase-server U: hbase-server Console output https://builds.apache.org/job/PreCommit-HBASE-Build/14138/console Powered by Apache Yetus 0.7.0 http://yetus.apache.org This message was automatically generated.
          hudson Hudson added a comment -

          Results for branch branch-2
          build #1173 on builds.a.o: -1 overall


          details (if available):

          +1 general checks
          – For more information see general report

          -1 jdk8 hadoop2 checks
          – For more information see jdk8 (hadoop2) report

          -1 jdk8 hadoop3 checks
          – For more information see jdk8 (hadoop3) report

          +1 source release artifact
          – See build output for details.

          +1 client integration test

          hudson Hudson added a comment - Results for branch branch-2 build #1173 on builds.a.o : -1 overall details (if available): +1 general checks – For more information see general report -1 jdk8 hadoop2 checks – For more information see jdk8 (hadoop2) report -1 jdk8 hadoop3 checks – For more information see jdk8 (hadoop3) report +1 source release artifact – See build output for details. +1 client integration test
          stack Michael Stack added a comment -

          yuzhihong@gmail.com Should this be in 2.1.x and 2.0.x?

          stack Michael Stack added a comment - yuzhihong@gmail.com Should this be in 2.1.x and 2.0.x?
          yuzhihong@gmail.com Ted Yu added a comment -

          I would think those two branches should have this fix.

          yuzhihong@gmail.com Ted Yu added a comment - I would think those two branches should have this fix.
          stack Michael Stack added a comment -

          Reopen for backport. I'll do it.

          stack Michael Stack added a comment - Reopen for backport. I'll do it.
          stack Michael Stack added a comment -

          Re-Resolving after pushing to branch-2.0 and branch-2.1

          stack Michael Stack added a comment - Re-Resolving after pushing to branch-2.0 and branch-2.1
          hudson Hudson added a comment -

          Results for branch master
          build #461 on builds.a.o: -1 overall


          details (if available):

          -1 general checks
          – For more information see general report

          +1 jdk8 hadoop2 checks
          – For more information see jdk8 (hadoop2) report

          -1 jdk8 hadoop3 checks
          – For more information see jdk8 (hadoop3) report

          +1 source release artifact
          – See build output for details.

          +1 client integration test

          hudson Hudson added a comment - Results for branch master build #461 on builds.a.o : -1 overall details (if available): -1 general checks – For more information see general report +1 jdk8 hadoop2 checks – For more information see jdk8 (hadoop2) report -1 jdk8 hadoop3 checks – For more information see jdk8 (hadoop3) report +1 source release artifact – See build output for details. +1 client integration test
          hudson Hudson added a comment -

          Results for branch branch-2.0
          build #742 on builds.a.o: -1 overall


          details (if available):

          +1 general checks
          – For more information see general report

          -1 jdk8 hadoop2 checks
          – For more information see jdk8 (hadoop2) report

          -1 jdk8 hadoop3 checks
          – For more information see jdk8 (hadoop3) report

          +1 source release artifact
          – See build output for details.

          hudson Hudson added a comment - Results for branch branch-2.0 build #742 on builds.a.o : -1 overall details (if available): +1 general checks – For more information see general report -1 jdk8 hadoop2 checks – For more information see jdk8 (hadoop2) report -1 jdk8 hadoop3 checks – For more information see jdk8 (hadoop3) report +1 source release artifact – See build output for details.
          hudson Hudson added a comment -

          Results for branch branch-2.1
          build #253 on builds.a.o: +1 overall


          details (if available):

          +1 general checks
          – For more information see general report

          +1 jdk8 hadoop2 checks
          – For more information see jdk8 (hadoop2) report

          +1 jdk8 hadoop3 checks
          – For more information see jdk8 (hadoop3) report

          +1 source release artifact
          – See build output for details.

          +1 client integration test

          hudson Hudson added a comment - Results for branch branch-2.1 build #253 on builds.a.o : +1 overall details (if available): +1 general checks – For more information see general report +1 jdk8 hadoop2 checks – For more information see jdk8 (hadoop2) report +1 jdk8 hadoop3 checks – For more information see jdk8 (hadoop3) report +1 source release artifact – See build output for details. +1 client integration test
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build HBase-1.3-IT #468 (See https://builds.apache.org/job/HBase-1.3-IT/468/)
          HBASE-21088 HStoreFile should be closed in HStore#hasReferences (apurtell: rev 36ef36ccc5babbf1e398097855ded3e2e1b9dc34)

          • (edit) hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build HBase-1.3-IT #468 (See https://builds.apache.org/job/HBase-1.3-IT/468/ ) HBASE-21088 HStoreFile should be closed in HStore#hasReferences (apurtell: rev 36ef36ccc5babbf1e398097855ded3e2e1b9dc34) (edit) hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
          hudson Hudson added a comment -

          Results for branch branch-1
          build #447 on builds.a.o: -1 overall


          details (if available):

          -1 general checks
          – For more information see general report

          -1 jdk7 checks
          – For more information see jdk7 report

          -1 jdk8 hadoop2 checks
          – For more information see jdk8 (hadoop2) report

          -1 source release artifact
          – See build output for details.

          hudson Hudson added a comment - Results for branch branch-1 build #447 on builds.a.o : -1 overall details (if available): -1 general checks – For more information see general report -1 jdk7 checks – For more information see jdk7 report -1 jdk8 hadoop2 checks – For more information see jdk8 (hadoop2) report -1 source release artifact – See build output for details.
          hudson Hudson added a comment -

          Results for branch branch-1.3
          build #455 on builds.a.o: -1 overall


          details (if available):

          +1 general checks
          – For more information see general report

          -1 jdk7 checks
          – For more information see jdk7 report

          -1 jdk8 hadoop2 checks
          – For more information see jdk8 (hadoop2) report

          +1 source release artifact
          – See build output for details.

          hudson Hudson added a comment - Results for branch branch-1.3 build #455 on builds.a.o : -1 overall details (if available): +1 general checks – For more information see general report -1 jdk7 checks – For more information see jdk7 report -1 jdk8 hadoop2 checks – For more information see jdk8 (hadoop2) report +1 source release artifact – See build output for details.
          hudson Hudson added a comment -

          Results for branch branch-1.4
          build #450 on builds.a.o: -1 overall


          details (if available):

          -1 general checks
          – For more information see general report

          -1 jdk7 checks
          – For more information see jdk7 report

          -1 jdk8 hadoop2 checks
          – For more information see jdk8 (hadoop2) report

          +1 source release artifact
          – See build output for details.

          hudson Hudson added a comment - Results for branch branch-1.4 build #450 on builds.a.o : -1 overall details (if available): -1 general checks – For more information see general report -1 jdk7 checks – For more information see jdk7 report -1 jdk8 hadoop2 checks – For more information see jdk8 (hadoop2) report +1 source release artifact – See build output for details.

          People

            yuzhihong@gmail.com Ted Yu
            yuzhihong@gmail.com Ted Yu
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: