comnetwork commented on a change in pull request #3699:
URL: https://github.com/apache/hbase/pull/3699#discussion_r718130209
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketProtoUtils.java
##########
@@ -127,19 +130,23 @@ private BucketProtoUtils() {
}
static ConcurrentHashMap<BlockCacheKey, BucketEntry> fromPB(
- Map<Integer, String> deserializers, BucketCacheProtos.BackingMap
backingMap)
+ Map<Integer, String> deserializers, BucketCacheProtos.BackingMap
backingMap,
+ Function<BucketEntry, Recycler> createRecycler)
throws IOException {
ConcurrentHashMap<BlockCacheKey, BucketEntry> result = new
ConcurrentHashMap<>();
for (BucketCacheProtos.BackingMapEntry entry : backingMap.getEntryList()) {
BucketCacheProtos.BlockCacheKey protoKey = entry.getKey();
BlockCacheKey key = new BlockCacheKey(protoKey.getHfilename(),
protoKey.getOffset(),
protoKey.getPrimaryReplicaBlock(), fromPb(protoKey.getBlockType()));
BucketCacheProtos.BucketEntry protoValue = entry.getValue();
+ // TODO:We use ByteBuffAllocator.HEAP here, because we could not get the
ByteBuffAllocator
Review comment:
@Apache9 ， thank you for review.
The original implementation here before my patch is
`ByteBufferAllocator.HEAP` ，but I think `RpcServer#bbAllocator` is better(when
`BucketEntry `is not restored from file, `BucketEntry#allocator` is
`RpcServer#bbAllocator`).
The problem here is we could not get `RpcServer#bbAllocator ` in
`BucketCache `ctor.There are two way to introduce `RpcServer#bbAllocator `,
but both are not very elegantly and the modification are somewhat entensive:
1. Changing the `BucketCache `ctor to add a `ByteBufferAllocator `parameter
and pass `RpcServer#bbAllocator `to
`BlockCacheFactory.createBlockCache(Configuration) `in
`HRegionServer(Configuration)`, which would make `BucketCache `and
`BlockCacheFactory `depends on `ByteBufferAllocator `, but `BucketCache
`indeed with no need for `ByteBufferAllocator ` unless it is restored from
file, so it seems not very elegantly.
2. Another way is set `BucketEntry#allocator` to null here and adding
`ByteBufferAllocator `parameter to `BlockCache#getBlock` method, passing
`CacheConfig.byteBuffAllocator` (which is also from`RpcServer#bbAllocator ` )
to it when invoking `BlockCache#getBlock` in `HFileReaderImpl.getCachedBlock`,
the influence for modifying `BlockCache#getBlock` is somwhat extensive and is
also a little strange, so it is also not very elegantly.
So I maintain `ByteBufferAllocator.HEAP` here as same as the
implementation before my patch. I think the first way is somewhat better if we
need to use `RpcServer#bbAllocator ` here.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketProtoUtils.java
##########
@@ -127,19 +130,23 @@ private BucketProtoUtils() {
}
static ConcurrentHashMap<BlockCacheKey, BucketEntry> fromPB(
- Map<Integer, String> deserializers, BucketCacheProtos.BackingMap
backingMap)
+ Map<Integer, String> deserializers, BucketCacheProtos.BackingMap
backingMap,
+ Function<BucketEntry, Recycler> createRecycler)
throws IOException {
ConcurrentHashMap<BlockCacheKey, BucketEntry> result = new
ConcurrentHashMap<>();
for (BucketCacheProtos.BackingMapEntry entry : backingMap.getEntryList()) {
BucketCacheProtos.BlockCacheKey protoKey = entry.getKey();
BlockCacheKey key = new BlockCacheKey(protoKey.getHfilename(),
protoKey.getOffset(),
protoKey.getPrimaryReplicaBlock(), fromPb(protoKey.getBlockType()));
BucketCacheProtos.BucketEntry protoValue = entry.getValue();
+ // TODO:We use ByteBuffAllocator.HEAP here, because we could not get the
ByteBuffAllocator
Review comment:
@Apache9 ， thank you for review.
The original implementation here before my patch is
`ByteBufferAllocator.HEAP` ，but I think `RpcServer#bbAllocator` is better(when
`BucketEntry `is not restored from file, `BucketEntry#allocator` is
`RpcServer#bbAllocator`).
The problem here is we could not get `RpcServer#bbAllocator ` in
`BucketCache `ctor.There are two way to introduce `RpcServer#bbAllocator `,
but both are not very elegantly and the modification are somewhat entensive:
1. Changing the `BucketCache `ctor to add a `ByteBufferAllocator `parameter
and pass `RpcServer#bbAllocator `to
`BlockCacheFactory.createBlockCache(Configuration) `in
`HRegionServer(Configuration)`, which would make `BucketCache `and
`BlockCacheFactory `depends on `ByteBufferAllocator `, but `BucketCache
`indeed with no need for `ByteBufferAllocator ` unless it is restored from
file, so it seems not very elegantly.
2. Another way is set `BucketEntry#allocator` to null here and adding
`ByteBufferAllocator `parameter to `BlockCache#getBlock` method, passing
`CacheConfig.byteBuffAllocator` (which is also from`RpcServer#bbAllocator ` )
to it when invoking `BlockCache#getBlock` in `HFileReaderImpl.getCachedBlock`,
the influence for modifying `BlockCache#getBlock` is somwhat extensive and is
also a little strange, so it is also not very elegantly.
So I maintain `ByteBufferAllocator.HEAP` here as same as the
implementation before my patch. I think the first way is somewhat better if we
need to use `RpcServer#bbAllocator ` here.
Using `ByteBufferAllocator.HEAP` or `RpcServer#bbAllocator` do not affect
correctness， but `RpcServer#bbAllocator` using DBB pool to alleviate Java GC.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketProtoUtils.java
##########
@@ -127,19 +130,23 @@ private BucketProtoUtils() {
}
static ConcurrentHashMap<BlockCacheKey, BucketEntry> fromPB(
- Map<Integer, String> deserializers, BucketCacheProtos.BackingMap
backingMap)
+ Map<Integer, String> deserializers, BucketCacheProtos.BackingMap
backingMap,
+ Function<BucketEntry, Recycler> createRecycler)
throws IOException {
ConcurrentHashMap<BlockCacheKey, BucketEntry> result = new
ConcurrentHashMap<>();
for (BucketCacheProtos.BackingMapEntry entry : backingMap.getEntryList()) {
BucketCacheProtos.BlockCacheKey protoKey = entry.getKey();
BlockCacheKey key = new BlockCacheKey(protoKey.getHfilename(),
protoKey.getOffset(),
protoKey.getPrimaryReplicaBlock(), fromPb(protoKey.getBlockType()));
BucketCacheProtos.BucketEntry protoValue = entry.getValue();
+ // TODO:We use ByteBuffAllocator.HEAP here, because we could not get the
ByteBuffAllocator
Review comment:
@Apache9 ， thank you for review.
The original implementation here before my patch is
`ByteBufferAllocator.HEAP` ，but I think `RpcServer#bbAllocator` is better(when
`BucketEntry `is not restored from file, `BucketEntry#allocator` is
`RpcServer#bbAllocator`).
The problem here is we could not get `RpcServer#bbAllocator ` in
`BucketCache `ctor.There are two way to introduce `RpcServer#bbAllocator `,
but both are not very elegantly and the modification are somewhat entensive:
1. Changing the `BucketCache `ctor to add a `ByteBufferAllocator `parameter
and pass `RpcServer#bbAllocator `to
`BlockCacheFactory.createBlockCache(Configuration) `in
`HRegionServer(Configuration)`, which would make `BucketCache `and
`BlockCacheFactory `depends on `ByteBufferAllocator `, but `BucketCache
`indeed with no need for `ByteBufferAllocator ` unless it is restored from
file, so it seems not very elegantly.
2. Another way is set `BucketEntry#allocator` to null here and adding
`ByteBufferAllocator `parameter to `BlockCache#getBlock` method, passing
`CacheConfig.byteBuffAllocator` (which is also from`RpcServer#bbAllocator ` )
to it when invoking `BlockCache#getBlock` in `HFileReaderImpl.getCachedBlock`,
the influence for modifying `BlockCache#getBlock` is somwhat extensive and is
also a little strange, so it is also not very elegantly.
So I maintain `ByteBufferAllocator.HEAP` here as same as the
implementation before my patch. I think the first way is somewhat better if we
need to use `RpcServer#bbAllocator ` here.
Using `ByteBufferAllocator.HEAP` or `RpcServer#bbAllocator` do not affect
correctness here， but `RpcServer#bbAllocator` using DBB pool to alleviate Java
GC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
