Copilot commented on code in PR #11702:
URL: https://github.com/apache/gravitino/pull/11702#discussion_r3426577012
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -195,6 +195,26 @@ public boolean invalidate(
});
}
+ /** {@inheritDoc} */
+ @Override
+ public boolean invalidateRelationEntry(
+ NameIdentifier ident, Entity.EntityType type,
SupportsRelationOperations.Type relType) {
+ checkArguments(ident, type, relType);
+ EntityCacheRelationKey key = EntityCacheRelationKey.of(ident, type,
relType);
+ return segmentedLock.withLock(
+ key,
+ () -> {
+ // Drop only the cached relation result and its index entry. Do NOT
cascade through the
+ // reverse index: it is shared across entities (e.g. all roles bound
to one metadata
+ // object), and evicting it here would drop other entities'
reverse-index mappings. An
+ // explicit cacheData.invalidate does not trigger the expiry
listener, so the reverse
+ // index is left intact.
+ cacheData.invalidate(key);
+ cacheIndex.remove(key.toString());
+ return true;
Review Comment:
invalidateRelationEntry() always returns true and uses
cacheData.invalidate(), which doesn't indicate whether anything was actually
removed. More importantly, explicit invalidation bypasses
invalidateExpiredItem(), so the reverse-index bookkeeping for this key is never
cleaned up, which can leak reverse-index state over time.
Consider removing the entry via cacheData.asMap().remove(key) (so you can
return an accurate boolean) and explicitly cleaning up reverse-index state for
this key without doing a BFS cascade.
##########
core/src/main/java/org/apache/gravitino/cache/SupportsRelationEntityCache.java:
##########
@@ -56,6 +56,24 @@ <E extends Entity & HasIdentifier> Optional<List<E>>
getIfPresent(
boolean invalidate(
NameIdentifier ident, Entity.EntityType type,
SupportsRelationOperations.Type relType);
+ /**
+ * Invalidates only the cached relation result for the given key, without
cascading through the
+ * reverse index.
+ *
+ * <p>Unlike {@link #invalidate(NameIdentifier, Entity.EntityType,
+ * SupportsRelationOperations.Type)}, this does not evict the reverse-index
mappings (which are
+ * shared across entities, e.g. all roles bound to one metadata object) or
other entities' caches.
+ * Use it when a relation result is known to be stale and the next read must
re-query the backend,
+ * but the shared reverse index must be preserved.
+ *
+ * @param ident the name identifier
+ * @param type the entity type
+ * @param relType the relation type
+ * @return true if the cache entry was removed
+ */
Review Comment:
The Javadoc for invalidateRelationEntry() promises that reverse-index
mappings are preserved, but the cache implementation needs to clean up
reverse-index bookkeeping for the invalidated relation key to avoid leaking
reverse-index state when entries are explicitly removed.
Please update this Javadoc to describe the intended semantics more precisely
(no BFS cascade to other entities, but reverse-index state for the invalidated
entry itself may be cleaned).
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -451,4 +456,36 @@ private <E extends Entity & HasIdentifier> void
batchPopulateRelationCache(
cache.put(sourceId, identType, relType, entityList);
}
}
+
+ /**
+ * Invalidates the {@link
SupportsRelationOperations.Type#METADATA_OBJECT_ROLE_REL} cache entries
+ * keyed by every securable object of the given role.
+ *
+ * <p>The relation cache is keyed by the metadata object
(catalog/schema/table/...), while a role
+ * mutation (grant/revoke/override/create) is invalidated from the role
side. The role-side BFS
+ * invalidation only reaches an object's relation cache entry when the role
had previously been
+ * cached as that object's binding role; a role that is newly granted access
to an object was
+ * never cached there, so without this explicit invalidation the stale role
list is served until
+ * the entry's TTL elapses.
+ */
+ private void invalidateMetadataObjectRoleRelationCache(Entity entity) {
+ if (!(entity instanceof RoleEntity)) {
+ return;
+ }
+ List<SecurableObject> securableObjects = ((RoleEntity)
entity).securableObjects();
+ if (securableObjects == null || securableObjects.isEmpty()) {
+ return;
+ }
+ String metalake = ((RoleEntity) entity).namespace().level(0);
+ for (SecurableObject securableObject : securableObjects) {
+ // Drop only the relation result entry for this object, not the shared
reverse index. The
+ // reverse index is shared across all roles bound to the object; a full
invalidate would
+ // cascade through it and evict the other roles' mappings. The next
listRolesByObject
+ // re-queries the backend and rebuilds both the entry and the reverse
index.
Review Comment:
This comment says invalidateRelationEntry() keeps the reverse index intact.
In practice, implementations should avoid a BFS cascade but still clean up
reverse-index bookkeeping for the invalidated relation entry itself (otherwise
explicit removals can leak reverse-index state).
Please reword the comment to reflect the intended behavior (no BFS cascade,
and the entry will be rebuilt on next read).
--
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]
