yuqi1129 commented on code in PR #13374:
URL: https://github.com/apache/gravitino/pull/13374#discussion_r4069698912
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -223,9 +251,21 @@ public <E extends Entity & HasIdentifier> List<E> batchGet(
return entity.isEmpty();
})
.toList();
+ // Unlike get(), the backend read is not done under the entries' cache
locks: holding one lock
+ // per key across a batch DB round trip would stall unrelated reads on the
same segments. So an
+ // invalidation can land between the read and the write-back. The epoch
sampled here detects
+ // that and skips the write-back, otherwise the stale copy would survive
until the TTL. The
+ // per-key lock makes the check and the put atomic against an invalidation
of the same key.
+ long epochBeforeRead = cacheInvalidationEpoch.get();
List<E> fetchEntities = backend.batchGet(noCacheIdents, entityType);
for (E entity : fetchEntities) {
- cache.put(entity);
+ cache.withCacheLock(
+ EntityCacheKey.of(entity.nameIdentifier(), entity.type()),
+ () -> {
+ if (cacheInvalidationEpoch.get() == epochBeforeRead) {
+ cache.put(entity);
+ }
+ });
Review Comment:
Addressed in bf77580044. For BaseEntityCache, batchGet now skips the per-key
lock when the entity type cannot be cached. It still calls cache.put, because
that method runs invalidateOnKeyChange even for non-cacheable entities. The new
ROLE test checks both that no key lock is taken and that the hook still runs.
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -223,9 +251,21 @@ public <E extends Entity & HasIdentifier> List<E> batchGet(
return entity.isEmpty();
})
.toList();
+ // Unlike get(), the backend read is not done under the entries' cache
locks: holding one lock
+ // per key across a batch DB round trip would stall unrelated reads on the
same segments. So an
+ // invalidation can land between the read and the write-back. The epoch
sampled here detects
+ // that and skips the write-back, otherwise the stale copy would survive
until the TTL. The
+ // per-key lock makes the check and the put atomic against an invalidation
of the same key.
Review Comment:
You are right: withGlobalLock does not wait for a key lock already held by
batchGet. Addressed in bf77580044 by checking the epoch again after cache.put
and invalidating the just-written key if an invalidation or clear happened
during the put. A deterministic test triggers clear from inside the put, after
the first epoch check, and verifies the stale value is absent afterward.
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -75,6 +76,11 @@ public class RelationalEntityStore
private EntityChangeLogCleaner entityChangeLogCleaner;
private EntityCache cache;
+ // Advanced before every cache invalidation and clear, whether local or
replayed from the change
+ // log, so that batchGet() can tell that an invalidation happened while its
backend read was in
+ // flight. Every invalidation must go through invalidateCache() /
clearCache() for this to hold.
+ private final AtomicLong cacheInvalidationEpoch = new AtomicLong();
Review Comment:
Addressed in bf77580044. The field comment now states that this
process-local counter covers only invalidations observed by this store. A
future SHARED cache without a local change-log listener must provide its own
distributed version check; the current counter cannot detect changes made by
another node.
--
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]
