Copilot commented on code in PR #8384:
URL: https://github.com/apache/hbase/pull/8384#discussion_r3452594288
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/NoOpCacheAccessService.java:
##########
@@ -306,4 +306,10 @@ public Optional<Boolean> shouldCacheBlock(BlockCacheKey
key, long maxTimestamp,
Objects.requireNonNull(conf, "conf must not be null");
return Optional.empty();
}
+
+ @Override
+ public long getCurrentSize() {
+ // TODO Auto-generated method stub
+ return 0;
+ }
Review Comment:
Remove the auto-generated TODO in this method; returning 0 is fine for the
no-op cache but the stub comment should not be left in production code.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAvoidCellReferencesIntoShippedBlocks.java:
##########
@@ -230,7 +233,9 @@ public void run() {
}
}
List<BlockCacheKey> cacheList = new ArrayList<>();
- Iterator<CachedBlock> iterator = cache.iterator();
+ @SuppressWarnings("unchecked")
+ Iterator<CachedBlock> iterator =
+ (Iterator<CachedBlock>)
CacheAccessServices.asCachedBlockIterable(cache).orElseThrow();
Review Comment:
This casts an Iterable<CachedBlock> to Iterator<CachedBlock>, which will
throw ClassCastException at runtime. Use the Iterable's iterator() instead (and
the `@SuppressWarnings` becomes unnecessary).
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TopologyBackedCacheAccessService.java:
##########
@@ -287,8 +287,8 @@ public long getFreeSize() {
}
/**
- * Returns aggregate occupied cache size across participating engines.
- * @return aggregate occupied cache size
+ * Returns aggregate total cache size across participating engines.
+ * @return aggregate total cache size
*/
@Override
Review Comment:
TopologyBackedCacheAccessService#getCurrentSize currently delegates to
getCurrentDataSize(), which reports only data-block occupancy and can
undercount total occupied cache size. Also, the size() Javadoc says "total
cache size" but the implementation sums CacheEngine#size(), which is defined as
occupied size in CacheEngine. These inconsistencies can lead to incorrect
metrics/diagnostics.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAvoidCellReferencesIntoShippedBlocks.java:
##########
@@ -383,7 +390,8 @@ public void run() {
Thread.sleep(1);
} catch (InterruptedException e1) {
}
- iterator = cache.iterator();
+ iterator = (Iterator<CachedBlock>)
CacheAccessServices.asCachedBlockIterable(cache)
+ .orElseThrow();
Review Comment:
Reassigning iterator repeats the same incorrect cast from
Iterable<CachedBlock> to Iterator<CachedBlock>, which will throw
ClassCastException. Use .iterator() on the Iterable returned by
asCachedBlockIterable(...).
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAvoidCellReferencesIntoShippedBlocks.java:
##########
@@ -364,10 +369,12 @@ public void testHBASE16372InReadPath() throws Exception {
try (ScanPerNextResultScanner scanner =
new
ScanPerNextResultScanner(TEST_UTIL.getAsyncConnection().getTable(tableName),
s)) {
Thread evictorThread = new Thread() {
+ @SuppressWarnings("unchecked")
@Override
public void run() {
List<BlockCacheKey> cacheList = new ArrayList<>();
- Iterator<CachedBlock> iterator = cache.iterator();
+ Iterator<CachedBlock> iterator = (Iterator<CachedBlock>)
CacheAccessServices
+ .asCachedBlockIterable(cache).orElseThrow();
Review Comment:
This also casts an Iterable<CachedBlock> to Iterator<CachedBlock>, which
will throw ClassCastException at runtime. Use .iterator() on the Iterable (and
the unchecked suppression is no longer needed).
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAvoidCellReferencesIntoShippedBlocks.java:
##########
@@ -407,7 +415,8 @@ public void run() {
while (true) {
newBlockRefCount = 0;
newCacheList.clear();
- iterator = cache.iterator();
+ iterator = (Iterator<CachedBlock>)
CacheAccessServices.asCachedBlockIterable(cache)
+ .orElseThrow();
Review Comment:
Same issue here: casting the Iterable<CachedBlock> to Iterator<CachedBlock>
will fail at runtime. Call .iterator() instead.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java:
##########
@@ -148,70 +152,73 @@ public void modifyConf(Configuration conf) {
}
public TestCacheOnWrite(CacheOnWriteType cowType, Compression.Algorithm
compress,
- boolean cacheCompressedData, BlockCache blockCache) {
+ boolean cacheCompressedData, CacheAccessService blockCache) {
this.cowType = cowType;
this.compress = compress;
this.cacheCompressedData = cacheCompressedData;
- this.blockCache = blockCache;
+ this.cache = blockCache;
testDescription = "[cacheOnWrite=" + cowType + ", compress=" + compress
+ ", cacheCompressedData=" + cacheCompressedData + "]";
LOG.info(testDescription);
}
- private static List<BlockCache> getBlockCaches() throws IOException {
+ private static List<CacheAccessService> getCacheServices() throws
IOException {
Configuration conf = TEST_UTIL.getConfiguration();
- List<BlockCache> blockcaches = new ArrayList<>();
+ List<CacheAccessService> caches = new ArrayList<>();
// default
- blockcaches.add(BlockCacheFactory.createBlockCache(conf));
+ caches.add(CacheAccessServiceTestFactory.fromConfiguration(conf));
// set LruBlockCache.LRU_HARD_CAPACITY_LIMIT_FACTOR_CONFIG_NAME to 2.0f
due to HBASE-16287
TEST_UTIL.getConfiguration().setFloat(LruBlockCache.LRU_HARD_CAPACITY_LIMIT_FACTOR_CONFIG_NAME,
2.0f);
// memory
- BlockCache lru = new LruBlockCache(128 * 1024 * 1024, 64 * 1024,
TEST_UTIL.getConfiguration());
- blockcaches.add(lru);
+ CacheAccessService lru =
+ CacheAccessServiceTestFactory.lru(128 * 1024 * 1024, 64 * 1024,
TEST_UTIL.getConfiguration());
+ caches.add(lru);
// bucket cache
FileSystem.get(conf).mkdirs(TEST_UTIL.getDataTestDir());
int[] bucketSizes =
{ INDEX_BLOCK_SIZE, DATA_BLOCK_SIZE, BLOOM_BLOCK_SIZE, 64 * 1024, 128 *
1024 };
- BlockCache bucketcache =
- new BucketCache("offheap", 128 * 1024 * 1024, 64 * 1024, bucketSizes, 5,
64 * 100, null);
- blockcaches.add(bucketcache);
- return blockcaches;
+ CacheAccessService bucketcache =
CacheAccessServiceTestFactory.bucket("offheap",
+ 128 * 1024 * 1024, 64 * 1024, bucketSizes, 5, 64 * 100, null);
+ caches.add(bucketcache);
+ return caches;
}
public static Stream<Arguments> parameters() throws IOException {
List<Arguments> params = new ArrayList<>();
- for (BlockCache blockCache : getBlockCaches()) {
+ for (CacheAccessService cache : getCacheServices()) {
for (CacheOnWriteType cowType : CacheOnWriteType.values()) {
for (Compression.Algorithm compress :
HBaseCommonTestingUtil.COMPRESSION_ALGORITHMS) {
for (boolean cacheCompressedData : new boolean[] { false, true }) {
- params.add(Arguments.of(cowType, compress, cacheCompressedData,
blockCache));
+ params.add(Arguments.of(cowType, compress, cacheCompressedData,
cache));
}
}
}
}
return params.stream();
}
- private void clearBlockCache(BlockCache blockCache) throws
InterruptedException {
- if (blockCache instanceof LruBlockCache) {
- ((LruBlockCache) blockCache).clearCache();
+ private void clearBlockCache(CacheAccessService cache) throws
InterruptedException {
+ // TODO: HBASE-30018 refactor later
+ BlockCache blockCache = ((BlockCacheBackedCacheAccessService)
cache).getBlockCache();
+ if (cache instanceof LruBlockCache) {
+ ((LruBlockCache) cache).clearCache();
} else {
Review Comment:
clearBlockCache checks `cache instanceof LruBlockCache` and then casts the
CacheAccessService to LruBlockCache. Since `cache` is a service wrapper (e.g.
BlockCacheBackedCacheAccessService), this condition will never be true and the
cast is unsafe if a non-wrapper service is ever passed. The check should be on
the underlying BlockCache instance instead.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java:
##########
@@ -490,7 +500,7 @@ private void
testCachingDataBlocksDuringCompactionInternals(boolean useTags,
// of testing
// BucketCache, we cannot verify block type as it is not stored in the
cache.
boolean cacheOnCompactAndNonBucketCache =
- cacheBlocksOnCompaction && !(blockCache instanceof BucketCache);
+ cacheBlocksOnCompaction && !(cache instanceof BucketCache);
Review Comment:
`cache instanceof BucketCache` will always be false here because `cache` is
a CacheAccessService (typically a wrapper), not the underlying legacy cache.
This changes the test logic so the "BucketCache" special-case is not applied,
which can make the assertions incorrect for bucket-backed services.
--
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]
