anoopsjohn commented on a change in pull request #3684:
URL: https://github.com/apache/hbase/pull/3684#discussion_r711109899
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
##########
@@ -1049,6 +1049,16 @@
public static final float HFILE_BLOCK_CACHE_SIZE_DEFAULT = 0.4f;
+ /**
+ * Configuration key for setting the fix size of the block size, default do
nothing and it should
+ * be explicitly set by user or only used within ClientSideRegionScanner. if
it's set less than
+ * current max on heap size, it overrides the max size of block cache
+ */
+ public static final String HBASE_BLOCK_CACHE_FIXED_SIZE_KEY =
Review comment:
We normally avoid adding these constants to Public HConstants,
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
##########
@@ -1049,6 +1049,16 @@
public static final float HFILE_BLOCK_CACHE_SIZE_DEFAULT = 0.4f;
+ /**
+ * Configuration key for setting the fix size of the block size, default do
nothing and it should
+ * be explicitly set by user or only used within ClientSideRegionScanner. if
it's set less than
+ * current max on heap size, it overrides the max size of block cache
+ */
+ public static final String HBASE_BLOCK_CACHE_FIXED_SIZE_KEY =
+ "hfile.block.cache.fixed.size";
Review comment:
You want to make a very specific name? That this is for on heap cache?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/client/ClientSideRegionScanner.java
##########
@@ -60,6 +62,15 @@ public ClientSideRegionScanner(Configuration conf,
FileSystem fs,
region = HRegion.newHRegion(CommonFSUtils.getTableDir(rootDir,
htd.getTableName()), null, fs,
conf, hri, htd, null);
region.setRestoredRegion(true);
+ // non RS process does not have a block cache, and this a client side
scanner,
+ // create one for MapReduce jobs to cache the INDEX block by setting to use
+ // IndexOnlyLruBlockCache and set a value to
HBASE_CLIENT_SCANNER_BLOCK_CACHE_SIZE_KEY
+ conf.set(BlockCacheFactory.BLOCKCACHE_POLICY_KEY, "IndexOnlyLru");
Review comment:
Or IndexOnlyLRU?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/ResizableBlockCache.java
##########
@@ -31,4 +31,13 @@
* @param size The max heap size.
*/
void setMaxSize(long size);
+
+ /**
+ * Check if block type is meta or index block
+ * @param blockType block type of a given HFile block
+ * @return true if block type is non-data block
+ */
+ default boolean isMetaBlock(BlockType blockType) {
+ return blockType.getCategory() != BlockType.BlockCategory.DATA;
Review comment:
On HFileBlock we will have a blockType mostly. But I believe there may
be some rare cases where this is not yet assigned.. So to be on safe side, can
we pls add a null check also here. I know this is an existing method only.
blockType != null && ...
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/ResizableBlockCache.java
##########
@@ -31,4 +31,13 @@
* @param size The max heap size.
*/
void setMaxSize(long size);
+
+ /**
+ * Check if block type is meta or index block
+ * @param blockType block type of a given HFile block
+ * @return true if block type is non-data block
+ */
+ default boolean isMetaBlock(BlockType blockType) {
Review comment:
isMetaBlock () is not just applicable for ResizableBlockCache. For any
BC, this is a useful stuff and so better will sit in BlockCache only?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/util/MemorySizeUtil.java
##########
@@ -216,6 +216,9 @@ public static float getBlockCacheHeapPercent(final
Configuration conf) {
public static long getOnHeapCacheSize(final Configuration conf) {
float cachePercentage =
conf.getFloat(HConstants.HFILE_BLOCK_CACHE_SIZE_KEY,
HConstants.HFILE_BLOCK_CACHE_SIZE_DEFAULT);
+ // only uses when it's ClientSideRegionScanner or user explicitly sets it
+ long fixedCacheSize =
conf.getLong(HConstants.HBASE_BLOCK_CACHE_FIXED_SIZE_KEY,
Review comment:
So here, we are exposing this fixed size for L1 cache as a generic
thing. At RS level also some one can use this. Well that is fine. On large
heap sized RS, this make sense also. But may be we have to do more checks I
fear?
Like in current way of both L1 size and memstore size as % of Xmx, we have
checks to make sure addition of wont exceed some 80% of Xmx.. Now when one is
used as fixed size and other as %, how we can make sure such things? Or else a
wrong configured RS can cause OOME! We can add this later in a follow up Jira
also. But if we are exposing this as a global thing, lets be sure of doing all
safety checks. Or else lets make this usable only at that special client
scanner level
--
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]
