jerryshao commented on code in PR #8297:
URL: https://github.com/apache/gravitino/pull/8297#discussion_r2306331001
##########
build.gradle.kts:
##########
@@ -204,6 +204,11 @@ allprojects {
"[-PtestMode=embedded] or [-PtestMode=deploy]."
)
}
+
+ // enable entity cache or not
+ val enableEntityCache = project.properties["enableEntityCache"] as?
String ?: "false"
+ param.systemProperty("enableEntityCache", enableEntityCache)
Review Comment:
Why do we need a configuration for test?
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -117,6 +118,23 @@ public <E extends Entity & HasIdentifier> void put(E e,
boolean overwritten)
}
@Override
+ public <E extends Entity & HasIdentifier> E update(
+ NameIdentifier ident,
+ Class<E> type,
+ Entity.EntityType entityType,
+ List<String> roles,
+ Function<E, E> updater)
+ throws IOException, NoSuchEntityException, EntityAlreadyExistsException {
+ roles.forEach(
+ roleName -> {
+ Namespace namespaceRole =
NamespaceUtil.ofRole(ident.namespace().level(0));
+ NameIdentifier nameIdentifierRole = NameIdentifier.of(namespaceRole,
roleName);
+ cache.invalidate(nameIdentifierRole, Entity.EntityType.ROLE);
+ });
+
+ return update(ident, type, entityType, updater);
+ }
+
Review Comment:
```
@Override
public <E extends Entity & HasIdentifier> List<E> listEntitiesByRelation(
Type relType, NameIdentifier nameIdentifier, Entity.EntityType
identType, boolean allFields)
throws IOException {
return cache.withCacheLock(
() -> {
Optional<List<E>> entities = cache.getIfPresent(relType,
nameIdentifier, identType);
if (entities.isPresent()) {
return entities.get();
}
List<E> backendEntities =
backend.listEntitiesByRelation(relType, nameIdentifier,
identType, allFields);
cache.put(nameIdentifier, identType, relType, backendEntities);
```
This code still have two issues, one is mentioned by @mchades about
`allFields` problem, another is mentioned by me about cache inconsistency
problem. Did you fix this problem also?
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -330,6 +349,12 @@ private <KEY, VALUE> Caffeine<KEY, VALUE>
newBaseBuilder(Config cacheConfig) {
return (Caffeine<KEY, VALUE>) builder;
}
+ private NameIdentifier getNameIdentifier(Entity entity) {
+ NameIdentifier nameIdent =
+ NameIdentifier.of(((HasIdentifier) entity).namespace(),
((HasIdentifier) entity).name());
Review Comment:
`HasIdentifier` has a method to get `NameIdentifier`; you don't have to
create it manually.
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -78,11 +79,21 @@ public class CaffeineEntityCache extends BaseEntityCache {
private static final Logger LOG =
LoggerFactory.getLogger(CaffeineEntityCache.class.getName());
private final ReentrantLock opLock = new ReentrantLock();
- /** Cache part */
- private final Cache<EntityCacheKey, List<Entity>> cacheData;
+ /** Cache data structure. cacheData[0] = Role1-KEY -> [catalog1, catalog2] */
+ private final Cache<EntityCacheRelationKey, List<Entity>> cacheData;
- /** Index part */
- private RadixTree<EntityCacheKey> cacheIndex;
+ /**
+ * Cache reverse index structure. cacheData[0] = Role1 -> [catalog1,
catalog2] cacheData[1] =
+ * catalog1 -> [tab1, tab2] reverseIndex[0] = catalog1-KEY -> Role1
reverseIndex[1] = catalog2-KEY
+ * -> Role1 reverseIndex[3] = tab1-KEY -> catalog1 reverseIndex[4] =
tab2-KEY -> catalog1
+ */
+ private ReverseIndexCache reverseIndex;
+
+ /**
+ * Cache Index structure. cacheData[0] = Role1 -> [catalog1, catalog2]
cacheData[1] = catalog1 ->
+ * [tab1, tab2] cacheIndex[0] = Role1-KEY -> Role1 cacheIndex[1] = Role1-KEY
-> Role1
Review Comment:
I don't quite understand the comment here, what is the meaning of
"cacheIndex[0] = Role1-KEY -> Role1 cacheIndex[1] = Role1-KEY -> Role1"?
##########
core/src/main/java/org/apache/gravitino/EntityStore.java:
##########
@@ -149,6 +149,14 @@ <E extends Entity & HasIdentifier> E update(
NameIdentifier ident, Class<E> type, EntityType entityType, Function<E,
E> updater)
throws IOException, NoSuchEntityException, EntityAlreadyExistsException;
+ <E extends Entity & HasIdentifier> E update(
Review Comment:
Yes, we'd better make it more general.
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -78,11 +79,21 @@ public class CaffeineEntityCache extends BaseEntityCache {
private static final Logger LOG =
LoggerFactory.getLogger(CaffeineEntityCache.class.getName());
private final ReentrantLock opLock = new ReentrantLock();
- /** Cache part */
- private final Cache<EntityCacheKey, List<Entity>> cacheData;
+ /** Cache data structure. cacheData[0] = Role1-KEY -> [catalog1, catalog2] */
+ private final Cache<EntityCacheRelationKey, List<Entity>> cacheData;
- /** Index part */
- private RadixTree<EntityCacheKey> cacheIndex;
+ /**
+ * Cache reverse index structure. cacheData[0] = Role1 -> [catalog1,
catalog2] cacheData[1] =
+ * catalog1 -> [tab1, tab2] reverseIndex[0] = catalog1-KEY -> Role1
reverseIndex[1] = catalog2-KEY
+ * -> Role1 reverseIndex[3] = tab1-KEY -> catalog1 reverseIndex[4] =
tab2-KEY -> catalog1
+ */
Review Comment:
Is "ROLE" the only case in which one entity includes others? I guess
`USER/GROUP` may have the same issues. Do you fix them all?
##########
core/src/main/java/org/apache/gravitino/cache/EntityCacheRelationKey.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.gravitino.cache;
+
+import java.util.Objects;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.SupportsRelationOperations;
+
+/** Key for Entity cache. */
+public class EntityCacheRelationKey extends EntityCacheKey {
+ private final SupportsRelationOperations.Type relationType;
+
+ /**
+ * Creates a new instance of {@link EntityCacheRelationKey} with the given
arguments.
+ *
+ * @param ident The identifier of the entity.
+ * @param type The type of the entity.
+ * @param relationType The type of the relation, it can be null.
+ * @return A new instance of {@link EntityCacheRelationKey}.
+ */
+ public static EntityCacheRelationKey of(
+ NameIdentifier ident, Entity.EntityType type,
SupportsRelationOperations.Type relationType) {
+ return new EntityCacheRelationKey(ident, type, relationType);
+ }
Review Comment:
Do we need to add the `allFields` to the key to solve the problem mentioned
by @mchades ?
--
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]
