Copilot commented on code in PR #9800:
URL: https://github.com/apache/gravitino/pull/9800#discussion_r2727417543
##########
core/src/test/java/org/apache/gravitino/storage/TestEntityStorageRelationCache.java:
##########
@@ -773,4 +773,180 @@ 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());
+
+ 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);
+
+ 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);
+
+ 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);
+
+ SupportsRelationOperations relationOperations =
(SupportsRelationOperations) store;
+
+ // 1. Fetch relation for Tag (Target side), it should be empty
+ // This populates the cache for (Tag, TAG, REL) with empty list
+ List<GenericEntity> entities =
+ relationOperations.listEntitiesByRelation(
+ SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL,
+ tag1.nameIdentifier(),
+ Entity.EntityType.TAG,
+ true);
+ Assertions.assertTrue(entities.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(
+ tag1.nameIdentifier(),
+ Entity.EntityType.TAG,
+
SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL));
+ Assertions.assertNotNull(cachedEntities);
+ Assertions.assertTrue(cachedEntities.isEmpty());
+ }
+ }
+
+ // 3. Add relation from Catalog (Source side)
+ relationOperations.updateEntityRelations(
+ SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL,
+ catalog.nameIdentifier(),
+ Entity.EntityType.CATALOG,
+ new NameIdentifier[] {tag1.nameIdentifier()},
+ new NameIdentifier[] {});
+
+ // 4. Fetch relation for Tag again, it should NOT be empty
+ entities =
+ relationOperations.listEntitiesByRelation(
+ SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL,
+ tag1.nameIdentifier(),
+ Entity.EntityType.TAG,
+ true);
+ Assertions.assertEquals(1, entities.size());
+ Assertions.assertEquals(catalog.name(), entities.get(0).name());
+
+ destroy(type);
+ }
+ }
Review Comment:
This test may fail due to a pre-existing bug in cache invalidation logic.
When updateEntityRelations is called at line 932-937, it should invalidate the
cache entry for the tag entity (tag1, TAG, TAG_METADATA_OBJECT_REL), but the
current implementation in RelationalEntityStore.updateEntityRelations
incorrectly uses srcEntityType (CATALOG) instead of the destination entity type
(TAG) when invalidating destination entities. This means the cached empty list
at (tag1, TAG, TAG_METADATA_OBJECT_REL) from step 1 will NOT be invalidated,
and step 4 will return the stale cached empty list instead of fetching the
updated relation from the backend, causing the assertion at line 946 to fail.
This issue wasn't caught by existing tests because they don't cache empty
lists first - but this PR enables caching empty lists, which exposes the bug.
Please verify that this test actually passes, and if it fails, the cache
invalidation logic in RelationalEntityStore.updateEntityRelations (lines
275-281) needs to be fixed to use the correct entity types for destination
entities.
--
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]
