Copilot commented on code in PR #9638:
URL: https://github.com/apache/gravitino/pull/9638#discussion_r2665023647
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -265,6 +266,14 @@ public <E extends Entity & HasIdentifier> List<E>
updateEntityRelations(
NameIdentifier[] destEntitiesToRemove)
throws IOException, NoSuchEntityException, EntityAlreadyExistsException {
cache.invalidate(srcEntityIdent, srcEntityType, relType);
+ for (NameIdentifier destToAdd : destEntitiesToAdd) {
+ cache.invalidate(destToAdd, srcEntityType, relType);
+ }
+
+ for (NameIdentifier destToRemove : destEntitiesToRemove) {
+ cache.invalidate(destToRemove, srcEntityType, relType);
Review Comment:
Same issue as above: incorrect entity type used for cache invalidation. When
invalidating cache for destination entities being removed, you must use their
actual entity type, not the source entity type.
For TAG_METADATA_OBJECT_REL with metadata object source, use EntityType.TAG.
For POLICY_METADATA_OBJECT_REL with metadata object source, use
EntityType.POLICY.
```suggestion
Entity.EntityType destEntityType = srcEntityType;
if (relType == Type.TAG_METADATA_OBJECT_REL) {
destEntityType = Entity.EntityType.TAG;
} else if (relType == Type.POLICY_METADATA_OBJECT_REL) {
destEntityType = Entity.EntityType.POLICY;
}
for (NameIdentifier destToAdd : destEntitiesToAdd) {
cache.invalidate(destToAdd, destEntityType, relType);
}
for (NameIdentifier destToRemove : destEntitiesToRemove) {
cache.invalidate(destToRemove, destEntityType, relType);
```
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -265,6 +266,14 @@ public <E extends Entity & HasIdentifier> List<E>
updateEntityRelations(
NameIdentifier[] destEntitiesToRemove)
throws IOException, NoSuchEntityException, EntityAlreadyExistsException {
cache.invalidate(srcEntityIdent, srcEntityType, relType);
+ for (NameIdentifier destToAdd : destEntitiesToAdd) {
+ cache.invalidate(destToAdd, srcEntityType, relType);
+ }
Review Comment:
Incorrect entity type used for cache invalidation. The code uses
`srcEntityType` for the destination entities, but this is incorrect. The cache
key includes the entity's own type, not the source type.
For TAG_METADATA_OBJECT_REL, if the source is a metadata object (like
CATALOG), the destinations are TAGs, so the entity type should be
EntityType.TAG, not the source entity type. Similarly, for
POLICY_METADATA_OBJECT_REL, destinations are POLICies (EntityType.POLICY).
The cache stores bidirectional relationships:
- `(catalog_ident, CATALOG, TAG_METADATA_OBJECT_REL)` -> List of TAG
entities
- `(tag_ident, TAG, TAG_METADATA_OBJECT_REL)` -> List of metadata object
entities
When invalidating the cache for destination TAGs, you need to use
EntityType.TAG, not the source entity type.
A proper fix would need to determine the correct entity type for each
destination entity based on the relation type. For TAG_METADATA_OBJECT_REL with
a metadata object source, destinations should use EntityType.TAG. For
POLICY_METADATA_OBJECT_REL with a metadata object source, destinations should
use EntityType.POLICY.
--
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]
