ss666 commented on code in PR #13152:
URL: https://github.com/apache/gravitino/pull/13152#discussion_r4012005046
##########
core/src/test/java/org/apache/gravitino/cache/TestCaffeineEntityCacheInvalidation.java:
##########
Review Comment:
the current test does not make the asynchronous removal listener execution
deterministic. As a result, the callback may execute before `cache.put(table)`,
in which case the buggy implementation would also pass because the subsequent
`put` recreates the index entry.
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -255,14 +255,22 @@ public <E, T extends Exception> E withCacheLock(
* Removes the expired entity from the cache. This method is a hook method
for the Cache, when an
* entry expires, it will call this method.
*
+ * <p>The removal callback may run after the same key has already been
reinserted (for example,
+ * an entry expires and its entity is re-fetched before the asynchronous
listener executes). In
+ * that case the index entry belongs to the new entry and must be kept,
otherwise a later
+ * parent-level invalidation can no longer discover the reinserted child.
The index entry is
+ * therefore removed only when the key is no longer present in {@code
cacheData}.
+ *
* @param key The key of the expired entity
*/
@Override
protected void invalidateExpiredItem(EntityCacheKey key) {
segmentedLock.withLock(
key,
() -> {
- cacheIndex.remove(key.toString());
+ if (cacheData.getIfPresent(key) == null) {
Review Comment:
consider using getIfPresentQuietly() instead of getIfPresent() to avoid
refreshing the expireAfterAccess timestamp as a side effect of the consistency
check.
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -255,14 +255,22 @@ public <E, T extends Exception> E withCacheLock(
* Removes the expired entity from the cache. This method is a hook method
for the Cache, when an
Review Comment:
the Java doc seems inconsistent with the implementation. This method does
not remove the entity from `cacheData`.
--
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]
