Copilot commented on code in PR #8522:
URL: https://github.com/apache/hbase/pull/8522#discussion_r3716657627
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDataTieringManager.java:
##########
@@ -234,7 +234,13 @@ public void testPrefetchWhenDataTieringEnabled() throws
IOException {
// Since we have one cold file among four files, only three should get
prefetched.
Optional<Map<String, Pair<String, Long>>> fullyCachedFiles =
blockCache.getFullyCachedFiles();
assertTrue(fullyCachedFiles.isPresent(), "We should get the fully cached
files from the cache");
- Waiter.waitFor(defaultConf, 10000, () -> fullyCachedFiles.get().size() ==
3);
+ try {
+ Waiter.waitFor(defaultConf, 10000, () -> fullyCachedFiles.get().size()
== 3);
+ } catch (Throwable e) {
+ System.out.println("fullyCachedFiles = " +
blockCache.getFullyCachedFiles());
+ System.out.println("cacheAccessService = " +
cacheConf.getCacheAccessService().getClass());
+ System.out.println("isCombinedBlockCache = " +
cacheConf.isCombinedBlockCache());
+ }
Review Comment:
The new try/catch around Waiter.waitFor catches Throwable and does not
rethrow, which can swallow JUnit assertion failures/timeouts and let the test
proceed with potentially invalid state. This can turn real failures into noisy
logs and a later, less-informative assertion (or even a false pass). Remove the
catch, or at least rethrow after collecting diagnostics.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java:
##########
@@ -353,7 +353,7 @@ private CacheAccessService
initCombinedBlockCacheBackedService(final String l1Ca
that.set(BLOCKCACHE_POLICY_KEY, l1CachePolicy);
CacheAccessService bc =
CacheAccessServiceTestFactory.fromConfiguration(that);
assertNotNull(bc);
- assertTrue(CacheAccessServiceTestFactory.blockCache(bc) instanceof
CombinedBlockCache);
+ // assertTrue(CacheAccessServiceTestFactory.blockCache(bc) instanceof
CombinedBlockCache);
Review Comment:
This assertion was commented out, which removes verification that the
configuration actually produced the expected combined-cache wiring. Since
CacheAccessServices#fromBlockCache now exposes CombinedBlockCache via a
topology-backed CacheAccessService, the test should assert the new expected
service type instead of disabling the check.
##########
hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestProcedureBypass.java:
##########
@@ -82,7 +82,7 @@ public static void tearDown() throws Exception {
procStore.stop(false);
procExecutor.join();
}
-
+
Review Comment:
This line is now a blank line containing trailing whitespace. Please remove
the trailing spaces to avoid noisy diffs and potential style-check failures.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessService.java:
##########
@@ -524,4 +539,16 @@ default Optional<Boolean> shouldCacheBlock(BlockCacheKey
key, long maxTimestamp,
Configuration conf) {
return Optional.empty();
}
+
+ /**
+ * Returns an Optional containing the map of files that have been fully
cached (all its blocks are
+ * present in the cache. This method may not be overridden by all
implementing classes. In such
+ * cases, the returned Optional will be empty.
+ * @return empty optional if this method is not supported, otherwise the
returned optional
+ * contains a map of all files that have been fully cached.
+ */
Review Comment:
Javadoc for getFullyCachedFiles has a mismatched parenthesis and unclear
wording ("all its blocks" + missing ')'). This makes the API contract harder to
read and can confuse implementers.
--
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]
