From 613b577b724dacffc86d4a55b82f2ccad461f69c Mon Sep 17 00:00:00 2001 From: Yutong Sean Date: Mon, 12 Jul 2021 17:00:55 +0800 Subject: [PATCH 1/8] HBASE-26083 In branch-2 & branch-1 L1 miss metric is always 0 when using CombinedBlockCache --- .../org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java index 92f5a47e6403..c4a1d177f0e5 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java @@ -78,7 +78,7 @@ public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, // we end up calling l2Cache.getBlock. // We are not in a position to exactly look at LRU cache or BC as BlockType may not be getting // passed always. - return l1Cache.containsBlock(cacheKey)? + return cacheKey.getBlockType().getCategory() == BlockCategory.DATA? l1Cache.getBlock(cacheKey, caching, repeat, updateCacheMetrics): l2Cache.getBlock(cacheKey, caching, repeat, updateCacheMetrics); } From 87997c7f928aa50e3b66a663ba55b4aaa2d9bbe1 Mon Sep 17 00:00:00 2001 From: Yutong Sean Date: Tue, 13 Jul 2021 11:25:29 +0800 Subject: [PATCH 2/8] Fix a logic bug. --- .../org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java index c4a1d177f0e5..3d00eea95cc4 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java @@ -78,7 +78,7 @@ public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, // we end up calling l2Cache.getBlock. // We are not in a position to exactly look at LRU cache or BC as BlockType may not be getting // passed always. - return cacheKey.getBlockType().getCategory() == BlockCategory.DATA? + return cacheKey.getBlockType().getCategory() != BlockCategory.DATA? l1Cache.getBlock(cacheKey, caching, repeat, updateCacheMetrics): l2Cache.getBlock(cacheKey, caching, repeat, updateCacheMetrics); } From 6c79aea19bc8b894fba9de7cea37f6d0371422e9 Mon Sep 17 00:00:00 2001 From: Yutong Sean Date: Tue, 13 Jul 2021 13:23:09 +0800 Subject: [PATCH 3/8] Add metric update in containsBlock method --- .../apache/hadoop/hbase/io/hfile/CombinedBlockCache.java | 2 +- .../org/apache/hadoop/hbase/io/hfile/LruBlockCache.java | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java index 3d00eea95cc4..92f5a47e6403 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java @@ -78,7 +78,7 @@ public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, // we end up calling l2Cache.getBlock. // We are not in a position to exactly look at LRU cache or BC as BlockType may not be getting // passed always. - return cacheKey.getBlockType().getCategory() != BlockCategory.DATA? + return l1Cache.containsBlock(cacheKey)? l1Cache.getBlock(cacheKey, caching, repeat, updateCacheMetrics): l2Cache.getBlock(cacheKey, caching, repeat, updateCacheMetrics); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java index d7f700a4e260..d782771e7667 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java @@ -557,7 +557,13 @@ public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repea */ @Override public boolean containsBlock(BlockCacheKey cacheKey) { - return map.containsKey(cacheKey); + boolean result = map.containsKey(cacheKey); + // If this block is not in LRU, this check time should be counted as a miss. + // Here is ugly, but the metric will be correct. :D + if (!result) { + stats.miss(false, cacheKey.isPrimary(), cacheKey.getBlockType()); + } + return result; } @Override From 7e07fd70dd8d96c3a3c9c8202f7212cd13f473d0 Mon Sep 17 00:00:00 2001 From: Yutong Sean Date: Tue, 13 Jul 2021 13:34:49 +0800 Subject: [PATCH 4/8] Refine the metrics update --- .../apache/hadoop/hbase/io/hfile/CombinedBlockCache.java | 7 ++++++- .../org/apache/hadoop/hbase/io/hfile/LruBlockCache.java | 8 +------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java index 92f5a47e6403..46cfe7ecaad4 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java @@ -78,7 +78,12 @@ public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, // we end up calling l2Cache.getBlock. // We are not in a position to exactly look at LRU cache or BC as BlockType may not be getting // passed always. - return l1Cache.containsBlock(cacheKey)? + boolean existInL1 = l1Cache.containsBlock(cacheKey); + if (!existInL1) { + // If the block does not exist in L1, the containsBlock should be counted as one miss. + l1Cache.getStats().miss(false, cacheKey.isPrimary(), cacheKey.getBlockType()); + } + return existInL1? l1Cache.getBlock(cacheKey, caching, repeat, updateCacheMetrics): l2Cache.getBlock(cacheKey, caching, repeat, updateCacheMetrics); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java index d782771e7667..d7f700a4e260 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java @@ -557,13 +557,7 @@ public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repea */ @Override public boolean containsBlock(BlockCacheKey cacheKey) { - boolean result = map.containsKey(cacheKey); - // If this block is not in LRU, this check time should be counted as a miss. - // Here is ugly, but the metric will be correct. :D - if (!result) { - stats.miss(false, cacheKey.isPrimary(), cacheKey.getBlockType()); - } - return result; + return map.containsKey(cacheKey); } @Override From 11724fa4fbe401d4b3417a0891c842e87ac1ac29 Mon Sep 17 00:00:00 2001 From: Yutong Sean Date: Tue, 13 Jul 2021 13:40:15 +0800 Subject: [PATCH 5/8] Refine the metrics update --- .../org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java index 46cfe7ecaad4..aee1bbedc784 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java @@ -79,7 +79,7 @@ public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, // We are not in a position to exactly look at LRU cache or BC as BlockType may not be getting // passed always. boolean existInL1 = l1Cache.containsBlock(cacheKey); - if (!existInL1) { + if (!existInL1 && updateCacheMetrics) { // If the block does not exist in L1, the containsBlock should be counted as one miss. l1Cache.getStats().miss(false, cacheKey.isPrimary(), cacheKey.getBlockType()); } From 2f70542ff3615ac12e069b8e6ea1f4aa68462b14 Mon Sep 17 00:00:00 2001 From: Yutong Sean Date: Tue, 13 Jul 2021 14:19:27 +0800 Subject: [PATCH 6/8] trigger QA --- .../org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java | 1 + 1 file changed, 1 insertion(+) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java index aee1bbedc784..f7b27befbd8c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java @@ -83,6 +83,7 @@ public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, // If the block does not exist in L1, the containsBlock should be counted as one miss. l1Cache.getStats().miss(false, cacheKey.isPrimary(), cacheKey.getBlockType()); } + return existInL1? l1Cache.getBlock(cacheKey, caching, repeat, updateCacheMetrics): l2Cache.getBlock(cacheKey, caching, repeat, updateCacheMetrics); From 192b212bfeca92eaceb11458771297eef1ccf779 Mon Sep 17 00:00:00 2001 From: Yutong Sean Date: Wed, 14 Jul 2021 12:12:04 +0800 Subject: [PATCH 7/8] Refine the L1 miss metric in combinedBC --- .../org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java index f7b27befbd8c..23f4cdb902ce 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java @@ -81,7 +81,7 @@ public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean existInL1 = l1Cache.containsBlock(cacheKey); if (!existInL1 && updateCacheMetrics) { // If the block does not exist in L1, the containsBlock should be counted as one miss. - l1Cache.getStats().miss(false, cacheKey.isPrimary(), cacheKey.getBlockType()); + l1Cache.getStats().miss(caching, cacheKey.isPrimary(), cacheKey.getBlockType()); } return existInL1? From ad2f086c94c637de7b60194e1f3b77140dfd43bb Mon Sep 17 00:00:00 2001 From: Yutong Sean Date: Thu, 15 Jul 2021 15:46:10 +0800 Subject: [PATCH 8/8] Refine the metric update condition --- .../org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java index 23f4cdb902ce..d3f2a73eb323 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java @@ -79,12 +79,12 @@ public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, // We are not in a position to exactly look at LRU cache or BC as BlockType may not be getting // passed always. boolean existInL1 = l1Cache.containsBlock(cacheKey); - if (!existInL1 && updateCacheMetrics) { + if (!existInL1 && updateCacheMetrics && !repeat) { // If the block does not exist in L1, the containsBlock should be counted as one miss. l1Cache.getStats().miss(caching, cacheKey.isPrimary(), cacheKey.getBlockType()); } - return existInL1? + return existInL1 ? l1Cache.getBlock(cacheKey, caching, repeat, updateCacheMetrics): l2Cache.getBlock(cacheKey, caching, repeat, updateCacheMetrics); }