Copilot commented on code in PR #8359: URL: https://github.com/apache/hbase/pull/8359#discussion_r3423024916
########## hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessServiceTestInstance.java: ########## @@ -0,0 +1,64 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.io.hfile.cache; + +import java.util.Objects; +import org.apache.hadoop.hbase.io.hfile.BlockCache; +import org.apache.yetus.audience.InterfaceAudience; + +/** + * Test helper that keeps a concrete {@link BlockCache} together with the {@link CacheAccessService} + * facade backed by that cache. + * <p> + * This is useful for tests that need to exercise production code through + * {@link CacheAccessService}, while still inspecting implementation-specific state through the + * concrete cache instance. + * </p> + * @param <T> concrete block cache type + */ [email protected] +public final class CacheAccessServiceTestInstance<T extends BlockCache> { + + private final T blockCache; + private final CacheAccessService cacheAccessService; + + CacheAccessServiceTestInstance(T blockCache) { + this.blockCache = Objects.requireNonNull(blockCache, "blockCache must not be null"); + this.cacheAccessService = CacheAccessServices.fromBlockCache(blockCache); + } Review Comment: `CacheAccessServiceTestInstance` is `public`, but its constructor is package-private. This prevents tests in other packages from wrapping an already-constructed `BlockCache` (which is a primary use case for this helper). Making the constructor public keeps the helper broadly usable without affecting current callers. ########## hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessServices.java: ########## @@ -56,6 +58,35 @@ public static CacheAccessService fromBlockCache(BlockCache blockCache) { Objects.requireNonNull(blockCache, "blockCache must not be null")); } + /** + * Creates a {@link CacheAccessService} from the block cache configuration. + * <p> + * This method is a compatibility factory for tests and transitional code paths that want to + * obtain a {@link CacheAccessService} directly from {@link Configuration}, while still using the + * existing {@link BlockCacheFactory} and legacy {@link BlockCache} implementations underneath. + * </p> + * <p> + * The method delegates block-cache construction to + * {@link BlockCacheFactory#createBlockCache(Configuration)}. If the legacy factory creates a + * {@link BlockCache}, the returned service is backed by that cache through + * {@link BlockCacheBackedCacheAccessService}. If the legacy factory does not create a cache, this + * method returns the disabled/no-op cache access service. + * </p> + * <p> + * This method does not introduce new cache-engine or topology-based runtime wiring. It is + * intended only as a bridge while existing HBase tests and integration paths migrate from direct + * {@link BlockCache} usage to {@link CacheAccessService}. + * </p> + * @param conf configuration used by {@link BlockCacheFactory} + * @return cache access service created from the configured legacy block cache, or disabled when + * no block cache is configured + * @throws NullPointerException if {@code conf} is {@code null} + */ + public static CacheAccessService fromConfiguration(Configuration conf) { + Objects.requireNonNull(conf, "conf must not be null"); + return fromBlockCache(BlockCacheFactory.createBlockCache(conf)); + } Review Comment: `fromConfiguration` unconditionally wraps the result of `BlockCacheFactory.createBlockCache(conf)`, but that factory can return `null` when the on-heap cache is disabled (e.g. `hfile.block.cache.size=0`). This currently causes an unexpected `NullPointerException` instead of returning the documented disabled/no-op service. ########## hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestCacheAccessServiceTestFactory.java: ########## @@ -0,0 +1,215 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.io.hfile.cache; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.util.concurrent.Executor; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseConfiguration; +import org.apache.hadoop.hbase.io.hfile.LruAdaptiveBlockCache; +import org.apache.hadoop.hbase.io.hfile.LruBlockCache; +import org.apache.hadoop.hbase.io.hfile.TinyLfuBlockCache; +import org.apache.hadoop.hbase.io.hfile.bucket.BucketCache; +import org.apache.hadoop.hbase.testclassification.IOTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +/** + * Tests for {@link CacheAccessServiceTestFactory}. + */ +@Tag(IOTests.TAG) +@Tag(SmallTests.TAG) +public class TestCacheAccessServiceTestFactory { + + private static final long MAX_SIZE = 1024L * 1024L; + private static final long BUCKET_CACHE_SIZE = 64L * 1024L * 1024L; + private static final long BLOCK_SIZE = 4096L; + private static final Executor DIRECT_EXECUTOR = Runnable::run; + + @Test + void testDisabled() { + CacheAccessService service = CacheAccessServiceTestFactory.disabled(); + + assertNotNull(service); + assertFalse(service.isCacheEnabled()); + } + + @Test + void testFromBlockCacheRejectsNull() { + assertThrows(NullPointerException.class, + () -> CacheAccessServiceTestFactory.fromBlockCache(null)); + } + + @Test + void testFromConfigurationRejectsNull() { + assertThrows(NullPointerException.class, + () -> CacheAccessServiceTestFactory.fromConfiguration(null)); + } Review Comment: The new `fromConfiguration(Configuration)` helper is only covered for null-rejection. It would be useful to add positive coverage for (1) cache-disabled configs returning a no-op service (e.g. `hfile.block.cache.size=0`), and (2) cache-enabled configs returning an enabled service. This guards the intended bridge behavior during test migrations. -- 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]
