snazy commented on code in PR #1339:
URL: https://github.com/apache/polaris/pull/1339#discussion_r2035402942
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCache.java:
##########
@@ -255,104 +150,74 @@ public void setCacheMode(EntityCacheMode cacheMode) {
@Nonnull PolarisBaseEntity entityToValidate,
int entityMinVersion,
int entityGrantRecordsMinVersion) {
- long entityCatalogId = entityToValidate.getCatalogId();
long entityId = entityToValidate.getId();
- PolarisEntityType entityType = entityToValidate.getType();
- // first lookup the cache to find the existing cache entry
+ // First lookup the cache to find the existing cache entry
ResolvedPolarisEntity existingCacheEntry = this.getEntityById(entityId);
- // the caller's fetched entity may have come from a stale lookup byName;
we should consider
- // the existingCacheEntry to be the older of the two for purposes of
invalidation to make
- // sure when we replaceCacheEntry we're also removing the old name if it's
no longer valid
- EntityCacheByNameKey nameKey = new EntityCacheByNameKey(entityToValidate);
- ResolvedPolarisEntity existingCacheEntryByName =
this.getEntityByName(nameKey);
- if (existingCacheEntryByName != null
- && existingCacheEntry != null
- && isNewer(existingCacheEntry, existingCacheEntryByName)) {
- existingCacheEntry = existingCacheEntryByName;
+ // See if we need to load or refresh that entity
+ if (existingCacheEntry != null
+ && existingCacheEntry.getEntity().getEntityVersion() >=
entityMinVersion
+ && existingCacheEntry.getEntity().getGrantRecordsVersion()
+ >= entityGrantRecordsMinVersion) {
+ // Cache hit and cache entry is up to date, nothing to do.
+ return existingCacheEntry;
}
- // the new one to be returned
- final ResolvedPolarisEntity newCacheEntry;
-
- // see if we need to load or refresh that entity
- if (existingCacheEntry == null
- || existingCacheEntry.getEntity().getEntityVersion() < entityMinVersion
- || existingCacheEntry.getEntity().getGrantRecordsVersion() <
entityGrantRecordsMinVersion) {
+ // Either cache miss, dropped entity or stale entity. In either case,
invalidate and reload it.
+ // Do the refresh in a critical section based on the entity ID to avoid
race conditions.
+ Lock lock = this.locks.get(entityId);
+ try {
+ lock.lock();
+
+ // Lookup the cache again in case another thread has already invalidated
it.
+ existingCacheEntry = this.getEntityById(entityId);
+
+ // See if the entity has been refreshed concurrently
+ if (existingCacheEntry != null
+ && existingCacheEntry.getEntity().getEntityVersion() >=
entityMinVersion
+ && existingCacheEntry.getEntity().getGrantRecordsVersion()
+ >= entityGrantRecordsMinVersion) {
+ // Cache hit and cache entry is up to date now, exit
+ return existingCacheEntry;
+ }
- // the refreshed entity
- final ResolvedEntityResult refreshedCacheEntry;
+ // We are the first to act upon this entity id, invalidate it
+ this.cache.invalidate(new IdKey(entityId));
- // was not found in the cache?
- final PolarisBaseEntity entity;
- final List<PolarisGrantRecord> grantRecords;
- final int grantRecordsVersion;
- if (existingCacheEntry == null) {
- // try to load it
- refreshedCacheEntry =
- this.polarisMetaStoreManager.loadResolvedEntityById(
- callContext, entityCatalogId, entityId, entityType);
- if (refreshedCacheEntry.isSuccess()) {
- entity = refreshedCacheEntry.getEntity();
- grantRecords = refreshedCacheEntry.getEntityGrantRecords();
- grantRecordsVersion = refreshedCacheEntry.getGrantRecordsVersion();
- } else {
- return null;
- }
- } else {
- // refresh it
- refreshedCacheEntry =
- this.polarisMetaStoreManager.refreshResolvedEntity(
- callContext,
- existingCacheEntry.getEntity().getEntityVersion(),
- existingCacheEntry.getEntity().getGrantRecordsVersion(),
- entityType,
- entityCatalogId,
- entityId);
- if (refreshedCacheEntry.isSuccess()) {
- entity =
- (refreshedCacheEntry.getEntity() != null)
- ? refreshedCacheEntry.getEntity()
- : existingCacheEntry.getEntity();
- if (refreshedCacheEntry.getEntityGrantRecords() != null) {
- grantRecords = refreshedCacheEntry.getEntityGrantRecords();
- grantRecordsVersion = refreshedCacheEntry.getGrantRecordsVersion();
- } else {
- grantRecords = existingCacheEntry.getAllGrantRecords();
- grantRecordsVersion =
existingCacheEntry.getEntity().getGrantRecordsVersion();
- }
- } else {
- // entity has been purged, remove it
- this.removeCacheEntry(existingCacheEntry);
- return null;
- }
+ // Get the entity from the cache or reload it now that it has been
invalidated
+ EntityCacheLookupResult cacheLookupResult =
+ this.getOrLoadEntityById(
Review Comment:
> To be clear, it is possible that there are still unidentified race
conditions in the code. The fact that the cache has two different ways of
accessing the same entity is a very tricky design.
+100
--
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]
