diqiu50 commented on code in PR #12006:
URL: https://github.com/apache/gravitino/pull/12006#discussion_r3666474466
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -80,33 +85,34 @@ public class CaffeineEntityCache extends BaseEntityCache {
private static final Logger LOG =
LoggerFactory.getLogger(CaffeineEntityCache.class.getName());
+ /**
+ * Entity types that must not be cached by this implementation.
+ *
+ * <p>{@code USER}, {@code GROUP} and {@code ROLE} are materialized with
relation-derived data
+ * joined in at load time: a role carries its securable objects, and a
user/group carries its role
+ * names. A mutation on the entity itself invalidates its own key through
the write path, but this
+ * embedded data also goes stale through a mutation on a different entity.
For example, deleting
+ * or renaming a securable object changes a role's materialized form, and
deleting or renaming a
+ * role changes a user's/group's role names. Such a mutation touches neither
this entity's own key
+ * nor any hierarchy ancestor of it, so neither the write-path invalidation
nor the prefix cascade
+ * in {@link #invalidateHierarchy} would evict it; only the (now removed)
reverse index could.
+ * Caching them would therefore serve stale authorization data.
+ */
+ private static final Set<Entity.EntityType> NON_CACHEABLE_TYPES =
+ Sets.immutableEnumSet(
+ Entity.EntityType.USER, Entity.EntityType.GROUP,
Entity.EntityType.ROLE);
+
/** Segmented locking for better concurrency */
private final SegmentedLock segmentedLock;
/** Cache data structure. */
- private final Cache<EntityCacheRelationKey, List<Entity>> cacheData;
-
- /** Cache reverse index structure. */
- private ReverseIndexCache reverseIndex;
+ private final Cache<EntityCacheKey, Entity> cacheData;
- /** Cache Index structure. */
- private RadixTree<EntityCacheRelationKey> cacheIndex;
+ /** Prefix index over cache keys, used for cascading removal of descendant
entries. */
+ private RadixTree<EntityCacheKey> cacheIndex;
Review Comment:
cacheIndex is handled outside the lock in size().
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -30,34 +30,39 @@
import com.googlecode.concurrenttrees.radix.ConcurrentRadixTree;
import com.googlecode.concurrenttrees.radix.RadixTree;
import
com.googlecode.concurrenttrees.radix.node.concrete.DefaultCharArrayNodeFactory;
-import java.util.ArrayDeque;
-import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
-import java.util.Queue;
import java.util.Set;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
-import java.util.stream.Collectors;
-import org.apache.commons.lang3.ArrayUtils;
-import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.Config;
import org.apache.gravitino.Configs;
import org.apache.gravitino.Entity;
import org.apache.gravitino.HasIdentifier;
import org.apache.gravitino.NameIdentifier;
-import org.apache.gravitino.SupportsRelationOperations;
-import org.apache.gravitino.meta.GenericEntity;
import org.apache.gravitino.meta.ModelVersionEntity;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-/** This class implements the {@link org.apache.gravitino.cache.EntityCache}
using Caffeine */
+/**
+ * This class implements the {@link org.apache.gravitino.cache.EntityCache}
using Caffeine.
+ *
+ * <p>The cache stores one entry per entity, keyed by the entity's {@code
NameIdentifier} and type.
+ * A radix-tree prefix index over the cache keys implements cascading removal:
invalidating an
+ * entity also drops every cached descendant entry (e.g. invalidating a
catalog drops the cached
+ * schemas and tables under it).
+ *
+ * <p>Relation query results are NOT cached by this implementation; relation
and list operations
+ * always fall back to the {@code EntityStore}. Entity types whose
materialized form embeds
+ * relation-derived data ({@code USER}, {@code GROUP}, {@code ROLE}) are
excluded from caching
+ * entirely, because without relation tracking their entries could not be
invalidated when the
+ * referenced entities change.
+ */
public class CaffeineEntityCache extends BaseEntityCache {
Review Comment:
The function of withMultipleKeyCacheLock is unused
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -400,8 +318,13 @@ public void insertRelation(
boolean override)
throws IOException {
backend.insertRelation(relType, srcIdentifier, srcType, dstIdentifier,
dstType, override);
- cache.invalidate(srcIdentifier, srcType, relType);
- cache.invalidate(dstIdentifier, dstType, relType);
+ // Defensive: relation results are not cached, and no currently cached
entity type embeds
+ // relation-derived data (the only types that do — USER/GROUP/ROLE — are
excluded from the
+ // cache), so these invalidations are a no-op today. They are kept so that
if a cached type ever
+ // starts materializing this relation, its stale single-entity entry is
dropped on a relation
+ // write.
Review Comment:
I don't think this comment is accurate anymore. Specifically, *"so these
invalidations are a no-op today"* doesn't appear to be correct.
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -80,33 +85,34 @@ public class CaffeineEntityCache extends BaseEntityCache {
private static final Logger LOG =
LoggerFactory.getLogger(CaffeineEntityCache.class.getName());
+ /**
+ * Entity types that must not be cached by this implementation.
+ *
+ * <p>{@code USER}, {@code GROUP} and {@code ROLE} are materialized with
relation-derived data
+ * joined in at load time: a role carries its securable objects, and a
user/group carries its role
+ * names. A mutation on the entity itself invalidates its own key through
the write path, but this
+ * embedded data also goes stale through a mutation on a different entity.
For example, deleting
+ * or renaming a securable object changes a role's materialized form, and
deleting or renaming a
+ * role changes a user's/group's role names. Such a mutation touches neither
this entity's own key
+ * nor any hierarchy ancestor of it, so neither the write-path invalidation
nor the prefix cascade
+ * in {@link #invalidateHierarchy} would evict it; only the (now removed)
reverse index could.
+ * Caching them would therefore serve stale authorization data.
+ */
+ private static final Set<Entity.EntityType> NON_CACHEABLE_TYPES =
+ Sets.immutableEnumSet(
+ Entity.EntityType.USER, Entity.EntityType.GROUP,
Entity.EntityType.ROLE);
Review Comment:
NON_CACHEABLE_TYPES (USER/GROUP/ROLE) is a correctness constraint — caching
them serves stale authorization data. But it's a private field in
CaffeineEntityCache, while SupportsEntityStoreCache#put documents nothing about
it. Since this PR aims to make the SPI re-implementable for #11737, suggest
moving the check to BaseEntityCache.put and documenting it on the interface.
--
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]
