Copilot commented on code in PR #10186:
URL: https://github.com/apache/gravitino/pull/10186#discussion_r2882128277
##########
core/src/test/java/org/apache/gravitino/storage/TestEntityStorageRelationCache.java:
##########
@@ -773,4 +773,230 @@ void testPolicyRelationMultipleBindings(String type,
boolean enableCache) throws
destroy(type);
}
}
+
+ @ParameterizedTest
+ @MethodSource("storageProvider")
+ void testCacheInvalidationOnNewRelation(String type, boolean enableCache)
throws Exception {
+ Config config = Mockito.mock(Config.class);
+ Mockito.when(config.get(Configs.CACHE_ENABLED)).thenReturn(enableCache);
+ init(type, config);
Review Comment:
In this test,
`Mockito.when(config.get(Configs.CACHE_ENABLED)).thenReturn(enableCache);` is
effectively overridden by `init(type, config)`, because
`AbstractEntityStorageTest.init()` stubs `Configs.CACHE_ENABLED` to always
return `true`. As a result, the `enableCache` parameter doesn’t actually
control whether the store initializes with cache enabled, and the assertions
guarded by `if (enableCache ...)` will be skipped in the `enableCache=false`
cases even though caching is still on. Consider moving the `CACHE_ENABLED`
stubbing to *after* `init(type, config)` (and before
`EntityStoreFactory.createEntityStore(config)`), or updating `init()` to
respect the passed value so the parameterized matrix is meaningful.
```suggestion
init(type, config);
Mockito.when(config.get(Configs.CACHE_ENABLED)).thenReturn(enableCache);
```
##########
core/src/test/java/org/apache/gravitino/storage/TestEntityStorageRelationCache.java:
##########
@@ -773,4 +773,230 @@ void testPolicyRelationMultipleBindings(String type,
boolean enableCache) throws
destroy(type);
}
}
+
+ @ParameterizedTest
+ @MethodSource("storageProvider")
+ void testCacheInvalidationOnNewRelation(String type, boolean enableCache)
throws Exception {
+ Config config = Mockito.mock(Config.class);
+ Mockito.when(config.get(Configs.CACHE_ENABLED)).thenReturn(enableCache);
+ init(type, config);
+
+ AuditInfo auditInfo =
+
AuditInfo.builder().withCreator("creator").withCreateTime(Instant.now()).build();
+
+ try (EntityStore store = EntityStoreFactory.createEntityStore(config)) {
+ store.initialize(config);
+
+ BaseMetalake metalake =
+ createBaseMakeLake(RandomIdGenerator.INSTANCE.nextId(), "metalake",
auditInfo);
+ store.put(metalake, false);
+
+ CatalogEntity catalog =
+ createCatalog(
+ RandomIdGenerator.INSTANCE.nextId(),
+ NamespaceUtil.ofCatalog("metalake"),
+ "catalog",
+ auditInfo);
+ store.put(catalog, false);
+
+ SupportsRelationOperations relationOperations =
(SupportsRelationOperations) store;
+
+ // 1. Fetch relation, it should be empty
+ List<TagEntity> tags =
+ relationOperations.listEntitiesByRelation(
+ SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL,
+ catalog.nameIdentifier(),
+ Entity.EntityType.CATALOG,
+ true);
+ Assertions.assertTrue(tags.isEmpty());
+
+ // 2. Verify cache has empty list if cache is enabled
+ if (enableCache && store instanceof RelationalEntityStore) {
+ RelationalEntityStore relationalEntityStore = (RelationalEntityStore)
store;
+ if (relationalEntityStore.getCache() instanceof CaffeineEntityCache) {
+ CaffeineEntityCache cache = (CaffeineEntityCache)
relationalEntityStore.getCache();
+ List<Entity> cachedEntities =
+ cache
+ .getCacheData()
+ .getIfPresent(
+ EntityCacheRelationKey.of(
+ catalog.nameIdentifier(),
+ Entity.EntityType.CATALOG,
+
SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL));
+ Assertions.assertNotNull(cachedEntities);
+ Assertions.assertTrue(cachedEntities.isEmpty());
+ }
+ }
+
+ // 3. Create a tag and add relation
+ Namespace tagNamespace = NameIdentifierUtil.ofTag("metalake",
"tag1").namespace();
+ TagEntity tag1 =
+ TagEntity.builder()
+ .withId(RandomIdGenerator.INSTANCE.nextId())
+ .withNamespace(tagNamespace)
+ .withName("tag1")
+ .withAuditInfo(auditInfo)
+ .withProperties(Collections.emptyMap())
+ .build();
+ store.put(tag1, false);
+
+ relationOperations.updateEntityRelations(
+ SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL,
+ catalog.nameIdentifier(),
+ Entity.EntityType.CATALOG,
+ new NameIdentifier[] {tag1.nameIdentifier()},
+ new NameIdentifier[] {});
+
+ // 4. Fetch relation again, it should not be empty
+ tags =
+ relationOperations.listEntitiesByRelation(
+ SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL,
+ catalog.nameIdentifier(),
+ Entity.EntityType.CATALOG,
+ true);
+ Assertions.assertEquals(1, tags.size());
+ Assertions.assertEquals(tag1.name(), tags.get(0).name());
+
+ List<UserEntity> owners =
+ relationOperations.listEntitiesByRelation(
+ SupportsRelationOperations.Type.OWNER_REL,
+ catalog.nameIdentifier(),
+ Entity.EntityType.CATALOG,
+ true);
+ Assertions.assertTrue(owners.isEmpty());
+
+ if (enableCache && store instanceof RelationalEntityStore) {
+ RelationalEntityStore relationalEntityStore = (RelationalEntityStore)
store;
+ if (relationalEntityStore.getCache() instanceof CaffeineEntityCache) {
+ CaffeineEntityCache cache = (CaffeineEntityCache)
relationalEntityStore.getCache();
+ List<Entity> cachedOwners =
+ cache
+ .getCacheData()
+ .getIfPresent(
+ EntityCacheRelationKey.of(
+ catalog.nameIdentifier(),
+ Entity.EntityType.CATALOG,
+ SupportsRelationOperations.Type.OWNER_REL));
+ Assertions.assertNotNull(cachedOwners);
+ Assertions.assertTrue(cachedOwners.isEmpty());
+ }
+ }
+
+ UserEntity ownerUser =
+ createUserEntity(
+ RandomIdGenerator.INSTANCE.nextId(),
+ AuthorizationUtils.ofUserNamespace("metalake"),
+ "ownerUser",
+ auditInfo);
+ store.put(ownerUser, false);
+
+ relationOperations.insertRelation(
+ SupportsRelationOperations.Type.OWNER_REL,
+ catalog.nameIdentifier(),
+ Entity.EntityType.CATALOG,
+ ownerUser.nameIdentifier(),
+ Entity.EntityType.USER,
+ true);
+
+ owners =
+ relationOperations.listEntitiesByRelation(
+ SupportsRelationOperations.Type.OWNER_REL,
+ catalog.nameIdentifier(),
+ Entity.EntityType.CATALOG,
+ true);
+ Assertions.assertEquals(1, owners.size());
+ Assertions.assertEquals(ownerUser.name(), owners.get(0).name());
+
+ destroy(type);
+ }
+ }
+
+ @ParameterizedTest
+ @MethodSource("storageProvider")
+ void testCacheInvalidationOnNewRelationReverse(String type, boolean
enableCache)
+ throws Exception {
+ Config config = Mockito.mock(Config.class);
+ Mockito.when(config.get(Configs.CACHE_ENABLED)).thenReturn(enableCache);
+ init(type, config);
Review Comment:
Same as the previous test: `init(type, config)` re-stubs
`Configs.CACHE_ENABLED` to `true` (see `AbstractEntityStorageTest.init()`), so
`enableCache` doesn’t actually disable caching for this parameterized run. This
makes the `enableCache=false` cases misleading and can skip cache assertions
even though cache is enabled. Re-stub `CACHE_ENABLED` after `init(...)` (before
store creation) or adjust `init()` so the test truly runs in both modes.
```suggestion
init(type, config);
// Re-stub CACHE_ENABLED after init, as init(type, config) may override
this setting.
Mockito.when(config.get(Configs.CACHE_ENABLED)).thenReturn(enableCache);
```
--
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]
