[GitHub] [hbase] virajjasani commented on a diff in pull request #5239: HBASE-27867 Close the L1 victim handler race

["virajjasani (via GitHub)" <gi...@apache.org>]

virajjasani commented on code in PR #5239:
URL: https://github.com/apache/hbase/pull/5239#discussion_r1195879024


##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java:
##########
@@ -592,10 +593,27 @@ public int evictBlocksByHfileName(String hfileName) {
    * @return the heap size of evicted block
    */
   protected long evictBlock(LruCachedBlock block, boolean evictedByEvictionProcess) {
-    LruCachedBlock previous = map.remove(block.getCacheKey());
-    if (previous == null) {
+    // We need something effectively final for the lambda.
+    final AtomicBoolean evicted = new AtomicBoolean(false);
+    map.computeIfPresent(block.getCacheKey(), (k, v) -> {
+      // Run the victim handler before we remove the mapping in the L1 map. It must complete
+      // quickly because other removal or insertion operations can be blocked in the meantime.
+      if (evictedByEvictionProcess && victimHandler != null) {
+        victimHandler.cacheBlock(k, v.getBuffer());
+      }
+      // Decrease the block's reference count, and if refCount is 0, then it'll auto-deallocate. DO
+      // NOT move this up because if do that then the victimHandler may access the buffer with

Review Comment:
   nit: `because if do that` -> `because if we do that`



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java:
##########
@@ -592,10 +593,27 @@ public int evictBlocksByHfileName(String hfileName) {
    * @return the heap size of evicted block
    */
   protected long evictBlock(LruCachedBlock block, boolean evictedByEvictionProcess) {
-    LruCachedBlock previous = map.remove(block.getCacheKey());
-    if (previous == null) {
+    // We need something effectively final for the lambda.
+    final AtomicBoolean evicted = new AtomicBoolean(false);
+    map.computeIfPresent(block.getCacheKey(), (k, v) -> {
+      // Run the victim handler before we remove the mapping in the L1 map. It must complete
+      // quickly because other removal or insertion operations can be blocked in the meantime.
+      if (evictedByEvictionProcess && victimHandler != null) {
+        victimHandler.cacheBlock(k, v.getBuffer());
+      }

Review Comment:
   with computeIfPresent, i think updates that fall into some bucket of keys get blocked and not all updates IIRC, but yes some updates do get blocked so comment seems good



-- 
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: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org