Copilot commented on code in PR #8704:
URL: https://github.com/apache/hbase/pull/8704#discussion_r4135794203
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java:
##########
@@ -641,11 +641,21 @@ public void onConfigurationChange(Configuration conf) {
@Override
public void registerChildren(ConfigurationManager manager) {
- manager.registerObserver(blockCache);
+ if (blockCache != null) {
+ manager.registerObserver(blockCache);
+ }
+ if(cacheAccessService != null) {
+ manager.registerObserver(cacheAccessService);
+ }
}
@Override
public void deregisterChildren(ConfigurationManager manager) {
- manager.deregisterObserver(blockCache);
+ if (blockCache != null) {
+ manager.deregisterObserver(blockCache);
+ }
+ if(cacheAccessService != null) {
+ manager.deregisterObserver(cacheAccessService);
+ }
Review Comment:
The deregistration mirrors the duplicate registration above: the legacy
`blockCache` is owned and notified through `cacheAccessService`, so removing
both references is redundant and leaves the same wrapper/child propagation
relationship inconsistent. Deregister only `cacheAccessService` here.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java:
##########
@@ -641,11 +641,21 @@ public void onConfigurationChange(Configuration conf) {
@Override
public void registerChildren(ConfigurationManager manager) {
- manager.registerObserver(blockCache);
+ if (blockCache != null) {
+ manager.registerObserver(blockCache);
+ }
+ if(cacheAccessService != null) {
+ manager.registerObserver(cacheAccessService);
Review Comment:
Registering both objects leaves the legacy `BlockCache` as a second
observer. In the legacy constructor, `cacheAccessService` is already a
topology-backed service whose `BlockCacheBackedCacheEngine` wraps this same
`blockCache`, so every configuration reload invokes the block cache callback
twice. Register only `cacheAccessService` (and remove the matching direct
deregistration) so it remains the single propagation root.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java:
##########
@@ -468,4 +472,32 @@ public void testL1CapacityEvictionMovesBlockToL2() throws
Exception {
assertTrue(l2.getBlockCount() > initialL2BlockCount);
}
+
+ /**
+ * Verifies that CacheConfig registers its CacheAccessService as a
configuration child.
+ */
+ @Test
+ public void testRegistersCacheAccessServiceAsConfigurationChild1() {
Review Comment:
The trailing `1` makes this test name ambiguous and inconsistent with the
paired deregistration test; it looks like an accidental suffix rather than
behavior being tested. Rename it to
`testRegistersCacheAccessServiceAsConfigurationChild`.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestTopologyBackedCacheAccessService.java:
##########
@@ -522,6 +522,40 @@ void testGetBlockDoesNotNotifyTopologyOnMiss() {
verify(topology, never()).handleAccess(key, l2);
}
+ /**
+ * Verifies that configuration changes are propagated to the engine in a
single-tier topology.
+ */
+ @Test
+ public void testConfigurationChangePropagatedToSingleTierEngine() {
+ CacheEngine engine = mock(CacheEngine.class);
+ CacheTopology topology = new SingleTierTopology("single", engine);
+ TopologyBackedCacheAccessService service =
+ new TopologyBackedCacheAccessService(topology, new
DefaultHBaseCachePlacementAdmissionPolicy());
+ Configuration conf = new Configuration(false);
+
+ service.onConfigurationChange(conf);
+
+ verify(engine).onConfigurationChange(conf);
+ }
+
+ /**
+ * Verifies that configuration changes are propagated to every engine in a
tiered topology.
+ */
+ @Test
+ public void testConfigurationChangePropagatedToTieredEngines() {
+ CacheEngine l1 = mock(CacheEngine.class);
+ CacheEngine l2 = mock(CacheEngine.class);
+ CacheTopology topology = new TieredExclusiveTopology("tiered",l1, l2);
Review Comment:
This new constructor call omits the required space after the comma, unlike
the surrounding Java formatting (for example, line 533); fix the spacing so the
test conforms to the repository's source style.
--
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]
