Copilot commented on code in PR #8712:
URL: https://github.com/apache/gravitino/pull/8712#discussion_r2535027024
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -222,10 +282,12 @@ public <E extends Entity & HasIdentifier> void put(
segmentedLock.withLock(
entityCacheKey,
() -> {
- // We still need to cache the entities even if the list is empty, to
avoid cache
- // misses. Consider the scenario where a user queries for an
entity's relations and the
- // result is empty. If we don't cache this empty result, the next
query will still hit the
- // backend, this is not desired.
+ // Return directly if entities are empty. No need to put an empty
list to cache, we will
+ // use another PR to resolve the performance problem.
+ if (entities.isEmpty()) {
+ return;
+ }
+
Review Comment:
Returning early when `entities.isEmpty()` may cause performance issues. The
previous implementation cached empty lists to avoid repeated backend queries
when no relations exist. Without caching empty results, each query for
non-existent relations will hit the backend. The comment mentions "we will use
another PR to resolve the performance problem," but this creates a known
performance regression in this PR. Consider keeping the original behavior of
caching empty lists.
```suggestion
// Cache empty lists as well to avoid repeated backend queries for
non-existent relations.
```
##########
core/src/main/java/org/apache/gravitino/cache/ReverseIndexRules.java:
##########
@@ -142,4 +147,40 @@ public class ReverseIndexRules {
});
}
};
+
+ // Keep policies/tags to objects reverse index for metadata objects, so the
key are objects and
+ // the values are policies/tags.
+ public static final ReverseIndexCache.ReverseIndexRule
GENERIC_METADATA_OBJECT_REVERSE_RULE =
+ (entity, key, reverseIndexCache) -> {
+ // Name in GenericEntity contains no metalake.
+ GenericEntity genericEntity = (GenericEntity) entity;
+ EntityType type = entity.type();
+ if (genericEntity.name() != null) {
+ String[] levels = genericEntity.name().split("\\.");
+ String metadataName = key.identifier().namespace().levels()[0];
+ NameIdentifier objectsNameIdentifier =
+ NameIdentifier.of(ArrayUtils.addFirst(levels, metadataName));
+ reverseIndexCache.put(objectsNameIdentifier, type, key);
+ }
+ };
+
+ // Keep objects to policies reverse index for policy objects, so the key are
policies and the
+ // values are objects.
+ public static final ReverseIndexCache.ReverseIndexRule POLICY_REVERSE_RULE =
+ (entity, key, reverseIndexCache) -> {
+ PolicyEntity policyEntity = (PolicyEntity) entity;
+ NameIdentifier nameIdentifier =
+ NameIdentifier.of(policyEntity.namespace(), policyEntity.name());
+ reverseIndexCache.put(nameIdentifier, Entity.EntityType.POLICY, key);
+ };
+
+ // Keep objects to policies reverse index for tag objects, so the key are
tags and the
+ // values are objects.
+ public static final ReverseIndexCache.ReverseIndexRule TAG_REVERSE_RULE =
+ (entity, key, reverseIndexCache) -> {
+ TagEntity policyEntity = (TagEntity) entity;
+ NameIdentifier nameIdentifier =
+ NameIdentifier.of(policyEntity.namespace(), policyEntity.name());
Review Comment:
The variable name `policyEntity` is misleading and should be `tagEntity`
since this rule processes `TagEntity` objects, not `PolicyEntity` objects.
```suggestion
TagEntity tagEntity = (TagEntity) entity;
NameIdentifier nameIdentifier =
NameIdentifier.of(tagEntity.namespace(), tagEntity.name());
```
##########
core/src/main/java/org/apache/gravitino/cache/ReverseIndexRules.java:
##########
@@ -142,4 +147,40 @@ public class ReverseIndexRules {
});
}
};
+
+ // Keep policies/tags to objects reverse index for metadata objects, so the
key are objects and
+ // the values are policies/tags.
+ public static final ReverseIndexCache.ReverseIndexRule
GENERIC_METADATA_OBJECT_REVERSE_RULE =
+ (entity, key, reverseIndexCache) -> {
+ // Name in GenericEntity contains no metalake.
+ GenericEntity genericEntity = (GenericEntity) entity;
+ EntityType type = entity.type();
+ if (genericEntity.name() != null) {
+ String[] levels = genericEntity.name().split("\\.");
+ String metadataName = key.identifier().namespace().levels()[0];
+ NameIdentifier objectsNameIdentifier =
+ NameIdentifier.of(ArrayUtils.addFirst(levels, metadataName));
+ reverseIndexCache.put(objectsNameIdentifier, type, key);
+ }
+ };
+
+ // Keep objects to policies reverse index for policy objects, so the key are
policies and the
+ // values are objects.
+ public static final ReverseIndexCache.ReverseIndexRule POLICY_REVERSE_RULE =
+ (entity, key, reverseIndexCache) -> {
+ PolicyEntity policyEntity = (PolicyEntity) entity;
+ NameIdentifier nameIdentifier =
+ NameIdentifier.of(policyEntity.namespace(), policyEntity.name());
+ reverseIndexCache.put(nameIdentifier, Entity.EntityType.POLICY, key);
+ };
+
+ // Keep objects to policies reverse index for tag objects, so the key are
tags and the
Review Comment:
The comment incorrectly states "Keep objects to policies reverse index for
tag objects" when it should say "Keep objects to tags reverse index for tag
objects" or similar. The word "policies" should be "tags".
```suggestion
// Keep objects to tags reverse index for tag objects, so the key are tags
and the
```
--
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]
