anoopsjohn commented on a change in pull request #1801:
URL: https://github.com/apache/hbase/pull/1801#discussion_r432812650
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##
@@ -377,7 +377,7 @@ private MemStore getMemstore() {
* @param family The current column family.
*/
protected void createCacheConf(final ColumnFamilyDescriptor family) {
-this.cacheConf = new CacheConfig(conf, family, region.getBlockCache(),
+this.cacheConf = new CacheConfig(conf, family, region.getRegionInfo(),
region.getBlockCache(),
Review comment:
Ya this is some thing we can avoid
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java
##
@@ -205,6 +206,7 @@ public CacheConfig(Configuration conf,
ColumnFamilyDescriptor family, BlockCache
this.blockCache = blockCache;
this.byteBuffAllocator = byteBuffAllocator;
LOG.info("Created cacheConfig: " + this + (family == null ? "" : " for
family " + family) +
Review comment:
IMO better we should move this LOG from here to HStore.
protected void createCacheConf(final ColumnFamilyDescriptor family) {
this.cacheConf = new CacheConfig(conf, family, region.getBlockCache(),
region.getRegionServicesForStores().getByteBuffAllocator());
}
After the cacheConf is created. CacheConfig is having toString() with all
details. This way we can avoid passing the RegionInfo/regionName unnecessarily
to CacheConfig just for log.
In Master branch, we are not seeing the CacheConfig log with every
compaction because of a different constructor being used.
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java
##
@@ -245,7 +245,8 @@ public void testDisableCacheDataBlock() throws IOException {
.setBlockCacheEnabled(false)
.build();
-cacheConfig = new CacheConfig(conf, columnFamilyDescriptor, null,
ByteBuffAllocator.HEAP);
Review comment:
All these test changes will be not needed
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java
##
@@ -205,6 +206,7 @@ public CacheConfig(Configuration conf,
ColumnFamilyDescriptor family, BlockCache
this.blockCache = blockCache;
this.byteBuffAllocator = byteBuffAllocator;
LOG.info("Created cacheConfig: " + this + (family == null ? "" : " for
family " + family) +
+(hri == null ? "" : " in region " + hri.getRegionNameAsString()) +
" with blockCache=" + blockCache);
Review comment:
Here this blockCache will just give object identifier as no toString()
there. BlockCache will be single object within RS. So IMO there is no need to
log that with every Store create. Anyway if you do other comment, we will
avoid it as CacheConfig#toString() wont be having BlockCache object
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
