wchevreuil commented on code in PR #8469:
URL: https://github.com/apache/hbase/pull/8469#discussion_r3586551960
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -722,22 +727,48 @@ public Cacheable getBlock(BlockCacheKey key, boolean
caching, boolean repeat,
// When using file io engine persistent cache,
// the cache map state might differ from the actual cache. If we reach
this block,
// we should remove the cache key entry from the backing map
- backingMap.remove(key);
- fileNotFullyCached(key, bucketEntry);
+ failedBucketEntryRead = true;
LOG.debug("Failed to fetch block for cache key: {}.", key, hioex);
} catch (IOException ioex) {
LOG.error("Failed reading block " + key + " from bucket cache", ioex);
checkIOErrorIsTolerated();
} finally {
lock.readLock().unlock();
}
+ if (failedBucketEntryRead) {
+ removeFailedBucketEntry(bucketEntry);
+ }
}
if (!repeat && updateCacheMetrics) {
cacheStats.miss(caching, key.isPrimary(), key.getBlockType());
}
return null;
}
+ private void removeFailedBucketEntry(BucketEntry bucketEntry) {
+ BlockCacheKey cacheKey = findBackingMapKey(bucketEntry);
+ if (cacheKey == null) {
+ return;
+ }
+ bucketEntry.withWriteLock(offsetLock, () -> {
+ if (backingMap.remove(cacheKey, bucketEntry)) {
+ blockEvicted(cacheKey, bucketEntry, true, false);
+ }
+ return null;
+ });
+ }
+
+ private BlockCacheKey findBackingMapKey(BucketEntry bucketEntry) {
Review Comment:
Rather than iterating the whole backingMap, can we use similar logic from
getBlockForReference to resolve the reference key? Please make the common code
reusable.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -1798,70 +1829,110 @@ private void checkIOErrorIsTolerated() {
if (isCacheEnabled() && (now - ioErrorStartTimeTmp) >
this.ioErrorsTolerationDuration) {
LOG.error("IO errors duration time has exceeded " +
ioErrorsTolerationDuration
+ "ms, disabling cache, please check your IOEngine");
- disableCache();
+ disableCache(false);
Review Comment:
Why we wouldn't want to persist the backingMap here?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -722,22 +727,48 @@ public Cacheable getBlock(BlockCacheKey key, boolean
caching, boolean repeat,
// When using file io engine persistent cache,
// the cache map state might differ from the actual cache. If we reach
this block,
// we should remove the cache key entry from the backing map
- backingMap.remove(key);
- fileNotFullyCached(key, bucketEntry);
+ failedBucketEntryRead = true;
LOG.debug("Failed to fetch block for cache key: {}.", key, hioex);
} catch (IOException ioex) {
LOG.error("Failed reading block " + key + " from bucket cache", ioex);
checkIOErrorIsTolerated();
} finally {
lock.readLock().unlock();
}
+ if (failedBucketEntryRead) {
+ removeFailedBucketEntry(bucketEntry);
Review Comment:
We are modifying behaviour with this. Now, in the event of an io read error,
even if checkIOErrorIsTolerated ignores the error, we will evict the block?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -722,15 +764,17 @@ public Cacheable getBlock(BlockCacheKey key, boolean
caching, boolean repeat,
// When using file io engine persistent cache,
// the cache map state might differ from the actual cache. If we reach
this block,
// we should remove the cache key entry from the backing map
- backingMap.remove(key);
- fileNotFullyCached(key, bucketEntry);
Review Comment:
Thanks for clarifying.
--
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]
