Copilot commented on code in PR #9638:
URL: https://github.com/apache/gravitino/pull/9638#discussion_r2671427276
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -264,7 +265,21 @@ public <E extends Entity & HasIdentifier> List<E>
updateEntityRelations(
NameIdentifier[] destEntitiesToAdd,
NameIdentifier[] destEntitiesToRemove)
throws IOException, NoSuchEntityException, EntityAlreadyExistsException {
+
+ // We need to clear the cache of the source entity and all destination
entities being added or
+ // removed. This ensures that any subsequent reads will fetch the updated
relations from the
+ // backend. For example, if we are adding a tag to table, we need to
invalidate the cache for
+ // that table and the tag being added or removed. Otherwise, we might
return stale data if we
+ // list all tags for that table or all tables for that tag.
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:
The cache invalidation logic for destination entities is incorrect. When
invalidating the cache for destination entities being added or removed, the
code uses `srcEntityType` as the entity type parameter, but this is wrong.
For TAG_METADATA_OBJECT_REL relations:
- If srcEntityType is a metadata object type (e.g., CATALOG), the
destination entities are TAGs, so we should use EntityType.TAG
- If srcEntityType is TAG, the destination entities are metadata objects
(but we'd need to look up their types)
For POLICY_METADATA_OBJECT_REL relations:
- If srcEntityType is a metadata object type, the destination entities are
POLICYs, so we should use EntityType.POLICY
- If srcEntityType is POLICY, the destination entities are metadata objects
The correct entity type for the destination should be determined based on
the relation type and source entity type. For example, if we're adding tag1 to
catalog1, the invalidation for tag1 should use EntityType.TAG, not
EntityType.CATALOG.
```suggestion
// Determine the appropriate entity type to use for destination-side
cache invalidation.
// For TAG_METADATA_OBJECT_REL and POLICY_METADATA_OBJECT_REL, when the
source is a metadata
// object, the destination entities are TAGs or POLICYs respectively.
Entity.EntityType destEntityTypeForCache = srcEntityType;
if (relType == Type.TAG_METADATA_OBJECT_REL && srcEntityType !=
Entity.EntityType.TAG) {
destEntityTypeForCache = Entity.EntityType.TAG;
} else if (relType == Type.POLICY_METADATA_OBJECT_REL
&& srcEntityType != Entity.EntityType.POLICY) {
destEntityTypeForCache = Entity.EntityType.POLICY;
}
for (NameIdentifier destToAdd : destEntitiesToAdd) {
cache.invalidate(destToAdd, destEntityTypeForCache, relType);
}
for (NameIdentifier destToRemove : destEntitiesToRemove) {
cache.invalidate(destToRemove, destEntityTypeForCache, relType);
```
--
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]
