sunyuhan1998 commented on code in PR #11702:
URL: https://github.com/apache/gravitino/pull/11702#discussion_r3428190626
##########
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
+ */
+ boolean invalidateRelationEntry(
Review Comment:
**On (1) — calling `invalidateOnKeyChange` from `store.update`**: you're
right — that works. The blocker is (2): what `invalidateOnKeyChange` does
internally.
**On (2) — the BFS problem.** The key point is: `invalidate(ident, type,
relType)` does **not** just drop the one relation entry. It calls
`invalidateEntities`, which is a BFS over the **shared** reverse index +
cacheIndex:
https://github.com/apache/gravitino/blob/4ada1e9b6644aa4691a54f8350cfc80c1a6889fa/core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java#L457-L496
```
...
cacheData.invalidate(currentKey); //evicts entity *cache*, not just the
relation row
...
```
Starting from the object, the walk follows
`reverseIndex.getValuesForKeysStartingWith(objectIdent)` — i.e. **every role
bound to that object** — evicts each role's own entity cache, and removes the
whole `reverseIndex[objectIdent]` entry. So one grant reaches far beyond the
single relation row.
To make it concrete, this is exactly what `testInvalidRelationCache`
exercises, and I verified it empirically :
- `readRole` and `writeRole` both bind `fileset`. Putting each one builds
`reverseIndex["fileset:FILESET"] = [readRole, writeRole]` and caches each
role's own entity.
- After `delete(readRole)`, that reverse-index entry is `[writeRole]` (the
test asserts `size == 1` here).
- `store.put(readRole, true)` then re-adds readRole, so the entry is back
to `[writeRole, readRole]`.
- Now if the helper calls `invalidate(fileset, FILESET,
METADATA_OBJECT_ROLE_REL)`, the BFS walks the reverse index from `fileset`,
evicts **both** roles' entity caches, and deletes the whole
`reverseIndex["fileset:FILESET"]` entry.
- The test then does `store.get(readRole)`, which re-loads only readRole,
so the entry becomes `[readRole]` — writeRole is never re-loaded. The next
assertion expects `size == 2` but gets `1`, hence the failure.
So BFS is not a query-correctness bug per se (a re-query rebuilds), but it
(1) breaks the existing `testInvalidRelationCache` reverse-index invariant, and
(2) over-invalidates: granting one role evicts the entity caches of **every**
role bound to the same object — extra DB round-trips for them, and their
reverse-index mappings are gone until re-loaded.
`invalidateRelationEntry` avoids this precisely because it does **not**
call `invalidateEntities`: it does only `cacheData.invalidate(relationKey)` +
`reverseIndex.remove(relationKey)` (which trims just this key's own bookkeeping
in `entityToReverseIndexMap`/`reverseIndex`, no walking) + `cacheIndex.remove`.
No cascade → other roles' caches and the shared reverse-index entry stay intact.
--
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]
