Copilot commented on code in PR #11033:
URL: https://github.com/apache/gravitino/pull/11033#discussion_r3233365198
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java:
##########
@@ -95,6 +137,38 @@ public void loadRole(Runnable runnable) {
}
}
+ /**
+ * Per-request {@link UserUpdatedAt} dedup. Loader may return {@link
Optional#empty()} to cache
+ * the "user not found" outcome and avoid repeated DB lookups within a
single request.
+ */
+ public Optional<UserUpdatedAt> computeUserInfoIfAbsent(
+ String key, Function<String, Optional<UserUpdatedAt>> loader) {
+ return userInfoCache.computeIfAbsent(key, loader);
+ }
+
+ /**
+ * Per-request {@link GroupUpdatedAt} dedup. Loader may return {@link
Optional#empty()} to cache
+ * the "group not found" outcome and avoid repeated DB lookups within a
single request.
+ */
+ public Optional<GroupUpdatedAt> computeGroupInfoIfAbsent(
+ String key, Function<String, Optional<GroupUpdatedAt>> loader) {
+ return groupInfoCache.computeIfAbsent(key, loader);
+ }
+
+ /** Per-request name→id dedup. Loader must return a non-null id or throw. */
+ public Long computeMetadataIdIfAbsent(String key, Function<String, Long>
loader) {
+ return metadataIdCache.computeIfAbsent(key, loader);
+ }
+
+ /**
+ * Per-request metadataId→owner dedup. Loader returns {@link
Optional#empty()} when the object has
+ * no owner; the absent result is cached as well.
+ */
+ public Optional<OwnerInfo> computeOwnerIfAbsent(
+ Long metadataId, Function<Long, Optional<OwnerInfo>> loader) {
+ return ownerCache.computeIfAbsent(metadataId, loader);
+ }
Review Comment:
`ConcurrentHashMap.computeIfAbsent` throws a `NullPointerException` if the
loader returns `null` (including returning `null` instead of
`Optional.empty()`). Consider defensively wrapping these loaders to fail with a
clearer exception (e.g., `IllegalStateException` / precondition) so call-site
bugs don’t surface as opaque CHM NPEs.
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineGravitinoCache.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.gravitino.cache;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.Ticker;
+import java.util.Optional;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * A Caffeine-backed implementation of {@link GravitinoCache}. Supports
configurable TTL and maximum
+ * size.
+ *
+ * @param <K> the key type
+ * @param <V> the value type
+ */
+public class CaffeineGravitinoCache<K, V> implements GravitinoCache<K, V> {
+
+ private final Cache<K, V> cache;
+
+ /**
+ * Creates a new CaffeineGravitinoCache with the given TTL and maximum size.
+ *
+ * @param ttlMs the time-to-live in milliseconds for cache entries
(safety-net TTL)
+ * @param maxSize the maximum number of entries in the cache
+ */
+ public CaffeineGravitinoCache(long ttlMs, long maxSize) {
+ this(ttlMs, maxSize, Ticker.systemTicker());
+ }
+
+ CaffeineGravitinoCache(long ttlMs, long maxSize, Ticker ticker) {
+ this.cache =
+ Caffeine.newBuilder()
+ .expireAfterWrite(ttlMs, TimeUnit.MILLISECONDS)
+ .maximumSize(maxSize)
+ .ticker(ticker)
+ .build();
+ }
+
+ @Override
+ public Optional<V> getIfPresent(K key) {
+ V value = cache.getIfPresent(key);
+ return Optional.ofNullable(value);
+ }
+
+ @Override
+ public void put(K key, V value) {
+ cache.put(key, value);
+ }
+
+ @Override
+ public void invalidate(K key) {
+ cache.invalidate(key);
+ }
+
+ @Override
+ public void invalidateAll() {
+ cache.invalidateAll();
+ }
+
+ @Override
+ public void invalidateByPrefix(String prefix) {
+ // Prefix invalidation scans all keys. It is intended for infrequent
structural invalidations
+ // such as dropping or renaming an entity hierarchy, not for per-request
hot paths.
+ cache.asMap().keySet().removeIf(k -> k instanceof String && ((String)
k).startsWith(prefix));
+ }
+
+ @Override
+ public long size() {
+ cache.cleanUp();
Review Comment:
`size()` currently forces `cache.cleanUp()` on every call. If `size()` is
used in hot paths (metrics, debugging endpoints, etc.), this can add avoidable
overhead. Since the interface already documents an approximate size, consider
either (a) returning `estimatedSize()` without cleanup, or (b) splitting
cleanup into an explicit maintenance method so callers opt in.
##########
core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationRequestContext.java:
##########
@@ -101,4 +105,153 @@ public void testLoadRoleFailThenSuccessThenIgnored()
throws Exception {
context.loadRole(counter::incrementAndGet);
assertEquals(2, counter.get(), "After a successful loadRole, further calls
must be ignored.");
}
+
+ @Test
+ public void testComputeUserInfoIfAbsentDedupesLoaderInvocation() {
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ AtomicInteger loaderCalls = new AtomicInteger();
+ UserUpdatedAt expected = new UserUpdatedAt(42L, 1234L);
+
+ Optional<UserUpdatedAt> first =
+ context.computeUserInfoIfAbsent(
+ "ml::alice",
+ k -> {
+ loaderCalls.incrementAndGet();
+ return Optional.of(expected);
+ });
Review Comment:
`AuthorizationRequestContext` adds `computeGroupInfoIfAbsent(...)`, but this
test suite only exercises the user/metadataId/owner helpers. Adding a small
test for the group-path (both present and `Optional.empty()` caching) would
align coverage with the new functionality.
##########
core/src/test/java/org/apache/gravitino/cache/TestGravitinoCache.java:
##########
@@ -0,0 +1,315 @@
+/*
+ * 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.gravitino.cache;
+
+import com.github.benmanes.caffeine.cache.Ticker;
+import java.util.Optional;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/** Tests for {@link CaffeineGravitinoCache} and {@link NoOpsGravitinoCache}.
*/
+public class TestGravitinoCache {
+
+ @Test
+ void testCaffeinePutAndGet() {
+ CaffeineGravitinoCache<String, Long> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ try {
+ cache.put("key1", 100L);
+ cache.put("key2", 200L);
+
+ Optional<Long> val1 = cache.getIfPresent("key1");
+ Assertions.assertTrue(val1.isPresent());
+ Assertions.assertEquals(100L, val1.get());
+
+ Optional<Long> val2 = cache.getIfPresent("key2");
+ Assertions.assertTrue(val2.isPresent());
+ Assertions.assertEquals(200L, val2.get());
+
+ Optional<Long> missing = cache.getIfPresent("nonexistent");
+ Assertions.assertFalse(missing.isPresent());
+
+ Assertions.assertEquals(2, cache.size());
+ } finally {
+ cache.close();
+ }
+ }
+
+ @Test
+ void testCaffeineInvalidate() {
+ CaffeineGravitinoCache<String, String> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ try {
+ cache.put("a", "val-a");
+ cache.put("b", "val-b");
+
+ cache.invalidate("a");
+ Assertions.assertFalse(cache.getIfPresent("a").isPresent());
+ Assertions.assertTrue(cache.getIfPresent("b").isPresent());
+
+ Assertions.assertEquals(1, cache.size());
+ } finally {
+ cache.close();
+ }
+ }
+
+ @Test
+ void testCaffeineInvalidateAll() {
+ CaffeineGravitinoCache<String, Integer> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ try {
+ cache.put("x", 1);
+ cache.put("y", 2);
+ cache.put("z", 3);
+
+ cache.invalidateAll();
+ Assertions.assertEquals(0, cache.size());
+ Assertions.assertFalse(cache.getIfPresent("x").isPresent());
+ } finally {
+ cache.close();
+ }
+ }
+
+ @Test
+ void testCaffeineInvalidateByPrefix() {
+ CaffeineGravitinoCache<String, Long> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ try {
+ // Simulate hierarchical keys: metalake::catalog::schema::
+ cache.put("lake1::cat1::", 1L);
+ cache.put("lake1::cat1::s1::", 2L);
+ cache.put("lake1::cat1::s1::t1::TABLE", 3L);
+ cache.put("lake1::cat1::s1::t2::TABLE", 4L);
+ cache.put("lake1::cat1::s2::", 5L);
+ cache.put("lake1::cat2::", 6L);
+ cache.put("lake2::cat3::", 7L);
+
+ Assertions.assertEquals(7, cache.size());
+
+ // Drop catalog cat1 — should invalidate cat1 and all children
+ cache.invalidateByPrefix("lake1::cat1::");
+
+ Assertions.assertEquals(2, cache.size());
+ Assertions.assertFalse(cache.getIfPresent("lake1::cat1::").isPresent());
+
Assertions.assertFalse(cache.getIfPresent("lake1::cat1::s1::").isPresent());
+
Assertions.assertFalse(cache.getIfPresent("lake1::cat1::s1::t1::TABLE").isPresent());
+
Assertions.assertFalse(cache.getIfPresent("lake1::cat1::s1::t2::TABLE").isPresent());
+
Assertions.assertFalse(cache.getIfPresent("lake1::cat1::s2::").isPresent());
+
+ // cat2 and lake2 should be unaffected
+ Assertions.assertTrue(cache.getIfPresent("lake1::cat2::").isPresent());
+ Assertions.assertTrue(cache.getIfPresent("lake2::cat3::").isPresent());
+ } finally {
+ cache.close();
+ }
+ }
+
+ @Test
+ void testCaffeineInvalidateByPrefixLeaf() {
+ CaffeineGravitinoCache<String, Long> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ try {
+ cache.put("lake1::cat1::s1::t1::TABLE", 1L);
+ cache.put("lake1::cat1::s1::t2::TABLE", 2L);
+ cache.put("lake1::cat1::s1::f1::FILESET", 3L);
+
+ // Drop specific table — only t1 should be invalidated
+ cache.invalidateByPrefix("lake1::cat1::s1::t1::TABLE");
+
+ Assertions.assertEquals(2, cache.size());
+
Assertions.assertFalse(cache.getIfPresent("lake1::cat1::s1::t1::TABLE").isPresent());
+
Assertions.assertTrue(cache.getIfPresent("lake1::cat1::s1::t2::TABLE").isPresent());
+
Assertions.assertTrue(cache.getIfPresent("lake1::cat1::s1::f1::FILESET").isPresent());
+ } finally {
+ cache.close();
+ }
+ }
+
+ @Test
+ void testCaffeineOverwrite() {
+ CaffeineGravitinoCache<String, Long> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ try {
+ cache.put("k", 1L);
+ Assertions.assertEquals(1L, cache.getIfPresent("k").get());
+
+ cache.put("k", 2L);
+ Assertions.assertEquals(2L, cache.getIfPresent("k").get());
+
+ Assertions.assertEquals(1, cache.size());
+ } finally {
+ cache.close();
+ }
+ }
+
+ @Test
+ void testNoOpsCache() {
+ NoOpsGravitinoCache<String, Long> cache = new NoOpsGravitinoCache<>();
+ try {
+ cache.put("key1", 100L);
+ Assertions.assertFalse(cache.getIfPresent("key1").isPresent());
+ Assertions.assertEquals(0, cache.size());
+
+ // All operations are no-ops, should not throw
+ cache.invalidate("key1");
+ cache.invalidateAll();
+ cache.invalidateByPrefix("any");
+ } finally {
+ cache.close();
+ }
+ }
+
+ @Test
+ void testCaffeineWithNonStringKeys() {
+ CaffeineGravitinoCache<Long, String> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ try {
+ cache.put(1L, "role1");
+ cache.put(2L, "role2");
+ cache.put(3L, "role3");
+
+ Assertions.assertEquals("role1", cache.getIfPresent(1L).get());
+ Assertions.assertEquals(3, cache.size());
+
+ cache.invalidate(2L);
+ Assertions.assertFalse(cache.getIfPresent(2L).isPresent());
+ Assertions.assertEquals(2, cache.size());
+ } finally {
+ cache.close();
+ }
+ }
+
+ @Test
+ void testCaffeineInvalidateByPrefixIgnoresNonStringKeys() {
+ CaffeineGravitinoCache<Long, String> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ try {
+ cache.put(10L, "role10");
+ cache.put(11L, "role11");
+
+ cache.invalidateByPrefix("1");
+
+ Assertions.assertEquals(2, cache.size());
+ Assertions.assertEquals("role10", cache.getIfPresent(10L).get());
+ Assertions.assertEquals("role11", cache.getIfPresent(11L).get());
+ } finally {
+ cache.close();
+ }
+ }
+
+ @Test
+ void testCaffeineExpiresAfterWriteTtl() {
+ ManualTicker ticker = new ManualTicker();
+ CaffeineGravitinoCache<String, Long> cache = new
CaffeineGravitinoCache<>(50L, 1000L, ticker);
+ try {
+ cache.put("k", 1L);
+ Assertions.assertTrue(cache.getIfPresent("k").isPresent());
+
+ ticker.advance(51L, TimeUnit.MILLISECONDS);
+ Optional<Long> afterTtl = cache.getIfPresent("k");
+ Assertions.assertFalse(afterTtl.isPresent(), "Entry should have expired
after write TTL");
+ } finally {
+ cache.close();
+ }
+ }
+
+ @Test
+ void testCaffeineEvictsBeyondMaxSize() {
+ CaffeineGravitinoCache<Long, Long> cache = new
CaffeineGravitinoCache<>(60_000L, 5L);
+ try {
+ for (long i = 0; i < 50L; i++) {
+ cache.put(i, i);
+ }
+ // Caffeine eviction is asynchronous but bounded; size must respect
maxSize within slack
+ Assertions.assertTrue(
+ cache.size() <= 50L,
+ "Cache size must not exceed inserted count, but eviction should kick
in");
+ Assertions.assertTrue(
+ cache.size() <= 10L,
+ "Eviction should trim entries close to maxSize=5; observed: " +
cache.size());
+ } finally {
+ cache.close();
+ }
+ }
+
Review Comment:
This eviction assertion is likely to be flaky: Caffeine’s size
enforcement/maintenance can be asynchronous, and `estimatedSize()` may
temporarily exceed a tight bound right after many writes. To reduce flakiness,
consider using a retry/await loop until size converges, or assert on a weaker
invariant (e.g., that eviction eventually reduces size below some threshold)
with a bounded wait.
--
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]
