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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
