dimas-b commented on code in PR #1339: URL: https://github.com/apache/polaris/pull/1339#discussion_r2033776337
########## jcstress-tests/src/jcstress/java/org/apache/polaris/core/persistence/cache/EntityCacheGetByIdTest.java: ########## @@ -0,0 +1,151 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.cache; + +import static org.apache.polaris.core.entity.PolarisEntityType.CATALOG; +import static org.openjdk.jcstress.annotations.Expect.*; + +import org.apache.polaris.core.PolarisCallContext; +import org.openjdk.jcstress.annotations.*; +import org.openjdk.jcstress.infra.results.II_Result; +import org.openjdk.jcstress.infra.results.I_Result; + +public class EntityCacheGetByIdTest { + + @JCStressTest + @Description( + "Tests getOrLoadById is thread-safe. In this test, two actors are calling getOrLoadById " + + "twice on the same key. Each actor returns the version of the entity returned by the " + + "two calls to getOrLoadById. Expected behaviour is that the two actors get the same " + + "object twice, or for an object to be updated to a newer version between reads. " + + "But the cache should never go backward and server a stale version after a newer one " + + "has been observed as it is not allowed by `cacheNewEntry`.") + @Outcome.Outcomes({ + @Outcome(id = "1, 1", expect = ACCEPTABLE, desc = "Got the same object twice"), + @Outcome(id = "2, 2", expect = ACCEPTABLE, desc = "Got the same object twice"), + @Outcome(id = "1, 2", expect = ACCEPTABLE_INTERESTING, desc = "Got updated object"), + @Outcome(id = "2, 1", expect = FORBIDDEN, desc = "Got stale object after update"), + @Outcome(expect = UNKNOWN, desc = "Not sure what happened"), + }) + @State() + public static class WithoutArbiter { + private final PolarisCallContext context; + private final EntityCache entityCache; + + public WithoutArbiter() { + context = new PolarisCallContext(new FakeBasePersistence(), new FakePolarisDiagnostics()); + entityCache = new EntityCache(new FakeMetaStoreManager()); + } + + @Actor + public void actor1(II_Result result) { + result.r1 = + entityCache + .getOrLoadEntityById(context, 0L, FakeMetaStoreManager.CATALOG_ID, CATALOG) + .getCacheEntry() + .getEntity() + .getEntityVersion(); + result.r2 = + entityCache + .getOrLoadEntityById(context, 0L, FakeMetaStoreManager.CATALOG_ID, CATALOG) + .getCacheEntry() + .getEntity() + .getEntityVersion(); + } + + @Actor + public void actor2(II_Result result) { + result.r1 = + entityCache + .getOrLoadEntityById(context, 0L, FakeMetaStoreManager.CATALOG_ID, CATALOG) + .getCacheEntry() + .getEntity() + .getEntityVersion(); + result.r2 = + entityCache + .getOrLoadEntityById(context, 0L, FakeMetaStoreManager.CATALOG_ID, CATALOG) + .getCacheEntry() + .getEntity() + .getEntityVersion(); + } + } + + @JCStressTest + @Description( + "Tests getOrLoadById is thread-safe. In this test, two actors are calling getOrLoadById " + + "twice on the same key. The updates received by the actors are not checked as part of " + + "this test. Instead, an arbiter runs after the actors have performed their calls and " + + "checks the version of the entity that is in the cache. Expected behaviour is that " + + "at most two database calls happens. Thus only versions 1 and 2 are acceptable. Any " + + "other version indicates that the cache was updated even after both threads populated " + + "it with their own values.") + @Outcome.Outcomes({ + @Outcome(id = "1", expect = ACCEPTABLE, desc = "All cache calls happened in sequence"), + @Outcome(id = "2", expect = ACCEPTABLE_INTERESTING, desc = "Concurrent cache updates happened"), + @Outcome(expect = FORBIDDEN, desc = "Threads did not read their own writes"), + }) + @State() + public static class WithArbiter { + private final PolarisCallContext context; + private final EntityCache entityCache; + + public WithArbiter() { + context = new PolarisCallContext(new FakeBasePersistence(), new FakePolarisDiagnostics()); + entityCache = new EntityCache(new FakeMetaStoreManager()); + } + + @Actor + public void actor1() { + entityCache + .getOrLoadEntityById(context, 0L, FakeMetaStoreManager.CATALOG_ID, CATALOG) + .getCacheEntry() + .getEntity() + .getEntityVersion(); + entityCache + .getOrLoadEntityById(context, 0L, FakeMetaStoreManager.CATALOG_ID, CATALOG) + .getCacheEntry() + .getEntity() + .getEntityVersion(); + } + + @Actor + public void actor2() { + entityCache + .getOrLoadEntityById(context, 0L, FakeMetaStoreManager.CATALOG_ID, CATALOG) + .getCacheEntry() + .getEntity() + .getEntityVersion(); Review Comment: Are these three calls necessary? ########## jcstress-tests/src/jcstress/java/org/apache/polaris/core/persistence/cache/EntityCacheGetAndRefreshIfNeededTest.java: ########## @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.cache; + +import static org.openjdk.jcstress.annotations.Expect.*; + +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.entity.PolarisBaseEntity; +import org.openjdk.jcstress.annotations.*; +import org.openjdk.jcstress.infra.results.II_Result; + +@JCStressTest +@Description( + "Tests getAndRefreshIfNeeded is thread-safe. In this test, two actors are calling" + + " getAndRefreshIfNeeded twice on the same key. Each actor returns the version of the" + + " entity returned by the two calls to getAndRefreshIfNeeded. The method is invoked using" + + " a minimum entity version equal to 2. Expected behaviour is that the two actors get" + + " the same object twice, or for an object to be updated to a newer version between" + + " reads. But the cache should never go backward and server a stale version after a newer" Review Comment: `server` -> `serve`? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/IndexedCache.java: ########## @@ -0,0 +1,225 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.cache; Review Comment: Maybe move it to a dedicated package for clarity? e.g. `org.apache.polaris.caffeine`? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/IndexedCache.java: ########## @@ -0,0 +1,225 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.cache; + +import static com.google.common.base.Preconditions.checkState; +import static java.util.Objects.requireNonNull; + +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.Ticker; +import com.google.common.collect.Sets; +import com.google.common.util.concurrent.Striped; +import java.time.Duration; +import java.util.ArrayDeque; +import java.util.Comparator; +import java.util.Deque; +import java.util.LinkedHashSet; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.locks.Lock; +import java.util.function.Function; +import java.util.function.Supplier; + +// Source: +// https://github.com/ben-manes/caffeine/blob/master/examples/indexable/src/main/java/com/github/benmanes/caffeine/examples/indexable/IndexedCache.java Review Comment: I believe this file need a `CODE_COPIED_TO_POLARIS` comment. ########## 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: Parallel calls to `getOrLoadEntityById` are allowed without a lock... Why do we do this under a lock in this case? ########## jcstress-tests/src/jcstress/java/org/apache/polaris/core/persistence/cache/FakeMetaStoreManager.java: ########## @@ -0,0 +1,309 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.cache; + +import static java.util.Collections.emptyList; + +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.entity.*; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.apache.polaris.core.persistence.dao.entity.*; +import org.apache.polaris.core.storage.PolarisStorageActions; + +/** + * Build a fake meta store manager that returns a single catalog entity. Every time the catalog is + * returned, an incremented entity version id is used. this is in order to disambiguate the entity + * returned by the cache and verify whether the cache is thread safe. + * + * <p>Think of it as a poor-man's Mockito. It is not a real mock, but it is good enough for this + * test. More importantly, it is a lot faster to instantiate than Mockito. It allows the stress test + * to run up to 100x more iterations in the same amount of time. + * + * <p>The only implemented methods are `loadResolvedEntityById` and `loadResolvedEntityByName`. Any + * time they are invoked, regardless of their parameters, the same catalog is returned but with an + * increased entity version id. + */ +public class FakeMetaStoreManager implements PolarisMetaStoreManager { + public static final int CATALOG_ID = 42; + private final Supplier<ResolvedEntityResult> catalogSupplier; + + public FakeMetaStoreManager() { + final AtomicInteger versionCounter = new AtomicInteger(1); + this.catalogSupplier = + () -> { + int version = versionCounter.getAndIncrement(); + CatalogEntity catalog = + new CatalogEntity.Builder() + .setId(CATALOG_ID) + .setInternalProperties(Map.of()) + .setProperties(Map.of()) + .setName("test") + .setParentId(PolarisEntityConstants.getRootEntityId()) + .setEntityVersion(version) + .build(); + return new ResolvedEntityResult(catalog, version, emptyList()); + }; + } + + /** + * Utility method that ensures that catalogs creation is thread safe. + * + * @return a catalog entity with an incremented version id + */ + private synchronized ResolvedEntityResult nextResult() { Review Comment: Why does it need to be `synchronized`? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCache.java: ########## @@ -18,185 +18,74 @@ */ package org.apache.polaris.core.persistence.cache; -import com.github.benmanes.caffeine.cache.Cache; -import com.github.benmanes.caffeine.cache.Caffeine; -import com.github.benmanes.caffeine.cache.RemovalListener; +import com.google.common.util.concurrent.Striped; import jakarta.annotation.Nonnull; import jakarta.annotation.Nullable; -import java.util.AbstractMap; -import java.util.List; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.TimeUnit; +import java.time.Duration; +import java.util.Comparator; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.locks.Lock; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.entity.PolarisBaseEntity; import org.apache.polaris.core.entity.PolarisEntityType; -import org.apache.polaris.core.entity.PolarisGrantRecord; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.ResolvedPolarisEntity; import org.apache.polaris.core.persistence.dao.entity.ResolvedEntityResult; /** The entity cache, can be private or shared */ public class EntityCache { + private static final Comparator<ResolvedPolarisEntity> RESOLVED_POLARIS_ENTITY_COMPARATOR = Review Comment: Is it used? ########## jcstress-tests/src/jcstress/java/org/apache/polaris/core/persistence/cache/EntityCacheCoherenceTest.java: ########## @@ -0,0 +1,175 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.cache; + +import static org.apache.polaris.core.entity.PolarisEntityType.CATALOG; +import static org.apache.polaris.core.persistence.cache.FakeMetaStoreManager.CATALOG_ID; +import static org.openjdk.jcstress.annotations.Expect.*; + +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.persistence.ResolvedPolarisEntity; +import org.openjdk.jcstress.annotations.*; +import org.openjdk.jcstress.infra.results.LL_Result; + +public class EntityCacheCoherenceTest { + /** The cache key associated with the catalog that is returned by the fake meta store manager. */ + private static final EntityCacheByNameKey KEY = new EntityCacheByNameKey(CATALOG, "test"); + + @JCStressTest + @Description( + "Tests that the two caches inside EntityCache are coherent with one another. In this test, two" + + " actors are calling getOrLoadById and getOrLoadByName. The entities received by the" + + " actors are not checked as part of this test. Instead, an arbiter runs after the" + + " actors have performed their calls and checks the version of the entity that are in" + + " each of the two caches using the getter methods getEntityById and getEntityByName. " + + " Expected behaviour is that the two caches return the same entity version. Any" + + " discrepancy indicates that the caches are out of sync.") + @Outcome.Outcomes({ + @Outcome(id = "1, 1", expect = ACCEPTABLE, desc = "The caches are in sync"), + @Outcome(id = "2, 2", expect = ACCEPTABLE, desc = "The caches are in sync"), + @Outcome(id = "2, 1", expect = FORBIDDEN, desc = "The caches are out of sync"), Review Comment: What about `1, 2`? ########## jcstress-tests/src/jcstress/java/org/apache/polaris/core/persistence/cache/EntityCacheCoherenceTest.java: ########## @@ -0,0 +1,175 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.cache; + +import static org.apache.polaris.core.entity.PolarisEntityType.CATALOG; +import static org.apache.polaris.core.persistence.cache.FakeMetaStoreManager.CATALOG_ID; +import static org.openjdk.jcstress.annotations.Expect.*; + +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.persistence.ResolvedPolarisEntity; +import org.openjdk.jcstress.annotations.*; +import org.openjdk.jcstress.infra.results.LL_Result; + +public class EntityCacheCoherenceTest { + /** The cache key associated with the catalog that is returned by the fake meta store manager. */ + private static final EntityCacheByNameKey KEY = new EntityCacheByNameKey(CATALOG, "test"); + + @JCStressTest + @Description( + "Tests that the two caches inside EntityCache are coherent with one another. In this test, two" + + " actors are calling getOrLoadById and getOrLoadByName. The entities received by the" + + " actors are not checked as part of this test. Instead, an arbiter runs after the" + + " actors have performed their calls and checks the version of the entity that are in" + + " each of the two caches using the getter methods getEntityById and getEntityByName. " + + " Expected behaviour is that the two caches return the same entity version. Any" + + " discrepancy indicates that the caches are out of sync.") + @Outcome.Outcomes({ + @Outcome(id = "1, 1", expect = ACCEPTABLE, desc = "The caches are in sync"), + @Outcome(id = "2, 2", expect = ACCEPTABLE, desc = "The caches are in sync"), + @Outcome(id = "2, 1", expect = FORBIDDEN, desc = "The caches are out of sync"), + @Outcome(expect = UNKNOWN, desc = "Not sure what happened"), + }) + @State() + public static class UsingGetters { + private final PolarisCallContext context; + private final EntityCache entityCache; + + public UsingGetters() { + context = new PolarisCallContext(new FakeBasePersistence(), new FakePolarisDiagnostics()); + entityCache = new EntityCache(new FakeMetaStoreManager()); + } + + @Actor + public void actor1() { + entityCache + .getOrLoadEntityById(context, 0L, CATALOG_ID, CATALOG) + .getCacheEntry() + .getEntity() + .getEntityVersion(); + entityCache + .getOrLoadEntityByName(context, KEY) + .getCacheEntry() + .getEntity() + .getEntityVersion(); + } + + @Actor + public void actor2() { + entityCache + .getOrLoadEntityById(context, 0L, CATALOG_ID, CATALOG) + .getCacheEntry() + .getEntity() + .getEntityVersion(); + entityCache + .getOrLoadEntityByName(context, KEY) Review Comment: Why not call the getters in the reverse order too? -- 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]
