Details
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
Attachments
- 21088.v1.txt
- 1 kB
- Ted Yu
- 21088.v2.txt
- 1 kB
- Ted Yu
- 21088.v2.txt
- 1 kB
- Ted Yu
- 21088.v3.txt
- 0.9 kB
- Ted Yu
- 21088.v4.txt
- 1.0 kB
- Ted Yu
Activity
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?
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().
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.
-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 | |
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.
The procedure test failures were not related to the patch.
I ran TestAsyncRegionAdminApi, TestBlockEvictionFromClient and TestSpaceQuotas locally which passed.
+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 | |
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.
Ain't it loadStoreFiles that is opening the files? If so, your try/finally is in the wrong place?
As I explained at the end of this comment:
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).
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 }
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.
-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 | |
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.
-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 | |
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.
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
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
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.
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
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
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.
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.
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.
HBASE-21088This message was automatically generated.