jerryshao commented on code in PR #12006:
URL: https://github.com/apache/gravitino/pull/12006#discussion_r3689407105
##########
core/src/main/java/org/apache/gravitino/cache/BaseEntityCache.java:
##########
@@ -30,8 +32,31 @@
* An abstract class that provides a basic implementation for the {@link
EntityCache} interface.
* This class is abstract and cannot be instantiated directly, it is designed
to be a base class for
* other entity cache implementations.
+ *
+ * <p>This class enforces the non-cacheable entity type contract documented on
{@link
+ * SupportsEntityStoreCache#put(Entity)}: {@link #put(Entity)} is final and
drops non-cacheable
+ * entities before delegating to {@link #doPut(Entity)}, so no subclass can
accidentally cache them.
*/
public abstract class BaseEntityCache implements EntityCache {
+
+ /**
+ * Entity types that must never be cached, see {@link
SupportsEntityStoreCache#put(Entity)} for
+ * the contract.
+ *
+ * <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 a prefix cascade
+ * over the entity hierarchy would evict it. 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:
My feeling is that to deliberately define the cacheable types, rather than
the non-cacheable types would be better.
You solution will blindly cache the new type entities; if that entity
doesn't fit your current design, it will lead to an issue. So I think an
explicit list is better. After we fully validate the new type, we can add it to
the list explicitly.
##########
core/src/main/java/org/apache/gravitino/cache/BaseEntityCache.java:
##########
@@ -30,8 +32,31 @@
* An abstract class that provides a basic implementation for the {@link
EntityCache} interface.
* This class is abstract and cannot be instantiated directly, it is designed
to be a base class for
* other entity cache implementations.
+ *
+ * <p>This class enforces the non-cacheable entity type contract documented on
{@link
+ * SupportsEntityStoreCache#put(Entity)}: {@link #put(Entity)} is final and
drops non-cacheable
+ * entities before delegating to {@link #doPut(Entity)}, so no subclass can
accidentally cache them.
*/
public abstract class BaseEntityCache implements EntityCache {
+
+ /**
+ * Entity types that must never be cached, see {@link
SupportsEntityStoreCache#put(Entity)} for
+ * the contract.
+ *
+ * <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 a prefix cascade
+ * over the entity hierarchy would evict it. 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:
My feeling is that to deliberately define the cacheable types, rather than
the non-cacheable types would be better.
Your solution will blindly cache the new type entities; if that entity
doesn't fit your current design, it will lead to an issue. So I think an
explicit list is better. After we fully validate the new type, we can add it to
the list explicitly.
--
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]
