Copilot commented on code in PR #12042:
URL: https://github.com/apache/gravitino/pull/12042#discussion_r3712985852
##########
core/src/main/java/org/apache/gravitino/cache/BaseEntityCache.java:
##########
@@ -45,35 +45,25 @@ public abstract class BaseEntityCache implements
EntityCache {
* SupportsEntityStoreCache#put(Entity)} for the contract. New entity types
are excluded by
* default until their invalidation behavior has been validated.
*
- * <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.
+ * <p>Only self-contained entities are cacheable: a stale copy of one is at
worst cosmetically old
+ * (an old comment, property, or job status), never a wrong pointer, and
each can be invalidated
+ * with a single one-to-one key drop. Every other type is read straight from
the store.
Review Comment:
“At worst cosmetically old … job status” is an overstatement: stale job
status is not purely cosmetic and can affect user decisions/automation.
Consider rephrasing to something more precise like “stale but not a wrong
pointer / not relation-derived,” to avoid implying job staleness is harmless.
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -56,10 +54,9 @@
* 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 by {@link BaseEntityCache#put}, because without relation tracking
their entries could
- * not be invalidated when the referenced entities change.
+ * always fall back to the {@code EntityStore}. Only the self-contained
metadata objects listed in
+ * {@link BaseEntityCache#isCacheable} are cached; every other type
(user/group/role, model/model
+ * version, function, and operational entities) is read straight from the
{@code EntityStore}.
Review Comment:
The javadoc claims “operational entities” are read straight from the
`EntityStore`, but `BaseEntityCache`’s allowlist still includes
`EntityType.JOB` (which is typically operational). This is
misleading/internally inconsistent: either adjust the wording to exclude `JOB`
from “operational entities”, or explicitly call out which operational types (if
any) remain cacheable.
##########
core/src/main/java/org/apache/gravitino/cache/BaseEntityCache.java:
##########
@@ -45,35 +45,25 @@ public abstract class BaseEntityCache implements
EntityCache {
* SupportsEntityStoreCache#put(Entity)} for the contract. New entity types
are excluded by
* default until their invalidation behavior has been validated.
*
- * <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.
+ * <p>Only self-contained entities are cacheable: a stale copy of one is at
worst cosmetically old
+ * (an old comment, property, or job status), never a wrong pointer, and
each can be invalidated
+ * with a single one-to-one key drop. Every other type is read straight from
the store.
+ * User/group/role embed relation-derived data that a per-node cache cannot
invalidate;
+ * model/model version and function carry a load-bearing pointer that would
be silently wrong if
+ * served stale.
*/
private static final Set<Entity.EntityType> CACHEABLE_TYPES =
Sets.immutableEnumSet(
Entity.EntityType.METALAKE,
Entity.EntityType.CATALOG,
Entity.EntityType.SCHEMA,
Entity.EntityType.TABLE,
+ Entity.EntityType.TOPIC,
Entity.EntityType.VIEW,
- Entity.EntityType.COLUMN,
Entity.EntityType.FILESET,
- Entity.EntityType.TOPIC,
Entity.EntityType.TAG,
- Entity.EntityType.MODEL,
- Entity.EntityType.MODEL_VERSION,
Entity.EntityType.POLICY,
- Entity.EntityType.TABLE_STATISTIC,
- Entity.EntityType.JOB_TEMPLATE,
- Entity.EntityType.JOB,
- Entity.EntityType.AUDIT,
- Entity.EntityType.FUNCTION);
+ Entity.EntityType.JOB);
Review Comment:
The PR changes cache behavior for multiple types (notably excluding `MODEL`,
`MODEL_VERSION`, and `FUNCTION`), but the updated tests only explicitly cover
model/model-version (indirectly via `size()==0`) and `TAG`. To make regressions
less likely, add direct assertions for at least one additional excluded type
(e.g., `FUNCTION`) and/or explicitly assert `MODEL_VERSION` is not contained
(not only via `size()`), using the same `cache.put(...)` +
`cache.contains(...)` pattern as the new TAG test.
--
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]
