Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-22463 Some paths in HFileScannerImpl did not consider block#release which will exhaust the ByteBuffAllocator #257

Merged
merged 1 commit into from May 30, 2019

Conversation

@openinx
Copy link
Member

openinx commented May 28, 2019

No description provided.

@openinx openinx changed the title Some paths in HFileScannerImpl did not consider block#release which will exhaust the ByteBuffAllocator HBASE-22463 Some paths in HFileScannerImpl did not consider block#release which will exhaust the ByteBuffAllocator May 28, 2019
@Apache-HBase

This comment has been minimized.

Copy link

Apache-HBase commented May 28, 2019

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 55 Docker mode activated.
_ Prechecks _
+1 hbaseanti 0 Patch does not have any anti-patterns.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 8 new or modified test files.
_ HBASE-21879 Compile Tests _
0 mvndep 25 Maven dependency ordering for branch
+1 mvninstall 269 HBASE-21879 passed
+1 compile 75 HBASE-21879 passed
+1 checkstyle 85 HBASE-21879 passed
+1 shadedjars 281 branch has no errors when building our shaded downstream artifacts.
-1 findbugs 216 hbase-server in HBASE-21879 has 11 extant Findbugs warnings.
+1 javadoc 50 HBASE-21879 passed
_ Patch Compile Tests _
0 mvndep 15 Maven dependency ordering for patch
+1 mvninstall 261 the patch passed
+1 compile 78 the patch passed
+1 javac 78 the patch passed
-1 checkstyle 78 hbase-server: The patch generated 1 new + 86 unchanged - 3 fixed = 87 total (was 89)
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedjars 295 patch has no errors when building our shaded downstream artifacts.
+1 hadoopcheck 701 Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0.
+1 findbugs 274 the patch passed
+1 javadoc 58 the patch passed
_ Other Tests _
-1 unit 21086 hbase-server in the patch failed.
+1 unit 60 hbase-external-blockcache in the patch passed.
+1 asflicense 71 The patch does not generate ASF License warnings.
24174
Reason Tests
Failed junit tests hadoop.hbase.client.TestCloneSnapshotFromClientNormal
hadoop.hbase.master.TestMasterMetricsWrapper
hadoop.hbase.client.TestSnapshotTemporaryDirectoryWithRegionReplicas
hadoop.hbase.replication.TestReplicationKillSlaveRS
hadoop.hbase.replication.TestReplicationKillSlaveRSWithSeparateOldWALs
hadoop.hbase.client.TestFromClientSide3
hadoop.hbase.client.TestAdmin2
hadoop.hbase.master.procedure.TestTruncateTableProcedure
hadoop.hbase.tool.TestLoadIncrementalHFiles
hadoop.hbase.master.TestAssignmentManagerMetrics
hadoop.hbase.client.replication.TestReplicationAdminWithClusters
hadoop.hbase.master.procedure.TestProcedurePriority
hadoop.hbase.namespace.TestNamespaceAuditor
hadoop.hbase.replication.TestReplicationDisableInactivePeer
hadoop.hbase.client.TestAsyncTableAdminApi
hadoop.hbase.master.procedure.TestSCPWithReplicasWithoutZKCoordinated
hadoop.hbase.replication.TestReplicationSmallTestsSync
hadoop.hbase.client.TestFromClientSide
hadoop.hbase.master.procedure.TestSCPWithoutZKCoordinated
hadoop.hbase.replication.TestReplicationStatus
hadoop.hbase.client.TestFromClientSideWithCoprocessor
hadoop.hbase.client.TestSnapshotDFSTemporaryDirectory
hadoop.hbase.tool.TestSecureLoadIncrementalHFiles
hadoop.hbase.client.replication.TestReplicationAdmin
hadoop.hbase.replication.TestReplicationSmallTests
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-257/1/artifact/out/Dockerfile
GITHUB PR #257
Optional Tests dupname asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname Linux 0c865cb1fff9 4.4.0-138-generic #164-Ubuntu SMP Tue Oct 2 17:16:02 UTC 2018 x86_64 GNU/Linux
Build tool maven
Personality /testptch/patchprocess/precommit/personality/provided.sh
git revision HBASE-21879 / b673000
maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z)
Default Java 1.8.0_181
findbugs v3.1.11
findbugs https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-257/1/artifact/out/branch-findbugs-hbase-server-warnings.html
checkstyle https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-257/1/artifact/out/diff-checkstyle-hbase-server.txt
unit https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-257/1/artifact/out/patch-unit-hbase-server.txt
Test Results https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-257/1/testReport/
Max. process+thread count 5126 (vs. ulimit of 10000)
modules C: hbase-server hbase-external-blockcache U: .
Console output https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-257/1/console
Powered by Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

…ease which will exhaust the ByteBuffAllocator
@openinx openinx force-pushed the openinx:HBASE-21879 branch from b22201f to 8007264 May 28, 2019
@Apache-HBase

This comment has been minimized.

Copy link

Apache-HBase commented May 28, 2019

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 152 Docker mode activated.
_ Prechecks _
+1 hbaseanti 0 Patch does not have any anti-patterns.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 8 new or modified test files.
_ HBASE-21879 Compile Tests _
0 mvndep 25 Maven dependency ordering for branch
+1 mvninstall 268 HBASE-21879 passed
+1 compile 73 HBASE-21879 passed
+1 checkstyle 87 HBASE-21879 passed
+1 shadedjars 278 branch has no errors when building our shaded downstream artifacts.
-1 findbugs 181 hbase-server in HBASE-21879 has 11 extant Findbugs warnings.
+1 javadoc 45 HBASE-21879 passed
_ Patch Compile Tests _
0 mvndep 13 Maven dependency ordering for patch
+1 mvninstall 254 the patch passed
+1 compile 74 the patch passed
+1 javac 74 the patch passed
+1 checkstyle 78 hbase-server: The patch generated 0 new + 86 unchanged - 3 fixed = 86 total (was 89)
+1 checkstyle 9 The patch passed checkstyle in hbase-external-blockcache
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedjars 273 patch has no errors when building our shaded downstream artifacts.
+1 hadoopcheck 517 Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0.
+1 findbugs 219 the patch passed
+1 javadoc 44 the patch passed
_ Other Tests _
+1 unit 7887 hbase-server in the patch passed.
+1 unit 40 hbase-external-blockcache in the patch passed.
+1 asflicense 51 The patch does not generate ASF License warnings.
10702
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-257/2/artifact/out/Dockerfile
GITHUB PR #257
Optional Tests dupname asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname Linux dbe3f6b9dea3 4.4.0-131-generic #157~14.04.1-Ubuntu SMP Fri Jul 13 08:53:17 UTC 2018 x86_64 GNU/Linux
Build tool maven
Personality /testptch/patchprocess/precommit/personality/provided.sh
git revision HBASE-21879 / b673000
maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z)
Default Java 1.8.0_181
findbugs v3.1.11
findbugs https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-257/2/artifact/out/branch-findbugs-hbase-server-warnings.html
Test Results https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-257/2/testReport/
Max. process+thread count 4956 (vs. ulimit of 10000)
modules C: hbase-server hbase-external-blockcache U: .
Console output https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-257/2/console
Powered by Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

return;
}
// We don't have to keep ref to EXCLUSIVE type of block
if (this.curBlock != null && this.curBlock.usesSharedMemory()) {

This comment has been minimized.

Copy link
@anoopsjohn

anoopsjohn May 29, 2019

Contributor

There is a concern here. Even if the block is on an exclusive heap memory area, we will keep ref to that in this list. In a Phoenix Aggregation kind of use case where many blocks might get fetched and not immediately shipped, we are keeping the ref unwantedly here for longer time. This makes the GC not able to reclaim the heap memory area for the blocks. This might be a hidden bomb IMO. Its not good to remove the MemType. Lets create the block with memory type as EXCLUSIVE when the block data is on heap. The block might be coming from LRU cache or by fetching the block data from HDFS into heap memory area. When the block comes from off heap BC or if it is backed by a BB from the pool (While reading from HDFS, read into pooled BB) lets create the block with mem type as SHARED. Every block can have the retain and release method but let the EXCLUSIVE types do a noop here.

This comment has been minimized.

Copy link
@openinx

openinx May 29, 2019

Author Member

FIled an separate issue for this: https://issues.apache.org/jira/browse/HBASE-21879. Let finish that in an new one because I believe it's will be many changes , also can helop to make this PR forward.

This comment has been minimized.

Copy link
@ramkrish86

ramkrish86 May 30, 2019

Contributor

So already as part of this JIRA you removed the MEMTYPE and so even the bucket cache engines like FileIOEngine will remove the memtype and so all the blocks will go through the release() way by adding to prevBlocks and then getting released. The new PR #268 will remove the need for addition to prevBlocks as it is a noop. So now both bucket cache and read from HDFS both will create onheap/offheap blocks and the type of block will determine what it is .

Copy link
Contributor

anoopsjohn left a comment

High level one comment is added. Did not check the remaining part of the patch

return false;
}

// The first key in the current block 'seekToBlock' is greater than the given
// seekBefore key. We will go ahead by reading the next block that satisfies the
// given key. Return the current block before reading the next one.
seekToBlock.release();

This comment has been minimized.

Copy link
@anoopsjohn

anoopsjohn May 30, 2019

Contributor

This fix is there as part of some other issue fix right?

This comment has been minimized.

Copy link
@openinx

openinx May 30, 2019

Author Member

Yeah, similar to the issue: https://issues.apache.org/jira/browse/HBASE-22480. Discussed with binlijin before, his patch can apply to branch-2.x and need not to apply to master branch, once the HBASE-21879 get merged into master, then this bug will be fixed in this patch.

// Promote this to L1.
if (result != null) {
if (caching) {
if (result instanceof HFileBlock && ((HFileBlock) result).usesSharedMemory()) {

This comment has been minimized.

Copy link
@anoopsjohn

anoopsjohn May 30, 2019

Contributor

As part of the new issue, we have to handle here also.. Because when it is shared memory, we can not keep the block as is in LRU cache.

This comment has been minimized.

Copy link
@openinx

openinx May 30, 2019

Author Member

Here, the following cacheBlock in LruBlockCache, we have a asReferencedHeapBlock method which will deepClone the shared memory into a heap one and cache the heap one. so there we no need to do extra check & deepClone now, it's safe to remove this.

https://github.com/apache/hbase/pull/257/files/800726459ced6d868abcfd871aa75f0e60c6d275#diff-e300bc9c680e6ccde942561c4c0b5a3dR528

Copy link
Contributor

anoopsjohn left a comment

+1. Lets address the comment as part of already raised issue. Just asked another Q. Seems the fix as part of another issue is also here in this. Any way u will merge. Fine.

@openinx openinx merged commit 68c5129 into apache:HBASE-21879 May 30, 2019
asfgit pushed a commit that referenced this pull request May 31, 2019
…ease which will exhaust the ByteBuffAllocator (#257)
asfgit pushed a commit that referenced this pull request Jun 18, 2019
…ease which will exhaust the ByteBuffAllocator (#257)
asfgit pushed a commit that referenced this pull request Jun 24, 2019
…ease which will exhaust the ByteBuffAllocator (#257)
openinx added a commit to openinx/hbase that referenced this pull request Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.