This is an automated email from the ASF dual-hosted git repository.
yuqi4733 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 252b911be [#4246] improvement(core): Remove role local cache (#4901)
252b911be is described below
commit 252b911bedb6a055209613ae3841322a0e475629
Author: lwyang <yangli...@xiaomi.com>
AuthorDate: Thu Sep 12 20:00:08 2024 +0800
[#4246] improvement(core): Remove role local cache (#4901)
### What changes were proposed in this pull request?
Remove role local cache
### Why are the changes needed?
Fix: #4246
### Does this PR introduce _any_ user-facing change?
/
### How was this patch tested?
exist ut
Co-authored-by: Qi Yu <y...@datastrato.com>
---
.../main/java/org/apache/gravitino/Configs.java | 7 ---
.../authorization/AccessControlManager.java | 2 +-
.../gravitino/authorization/RoleManager.java | 56 +++-------------------
.../authorization/TestAccessControlManager.java | 5 --
4 files changed, 8 insertions(+), 62 deletions(-)
diff --git a/core/src/main/java/org/apache/gravitino/Configs.java
b/core/src/main/java/org/apache/gravitino/Configs.java
index fa65d399a..587d85ada 100644
--- a/core/src/main/java/org/apache/gravitino/Configs.java
+++ b/core/src/main/java/org/apache/gravitino/Configs.java
@@ -327,13 +327,6 @@ public class Configs {
ConfigConstants.NOT_BLANK_ERROR_MSG)
.create();
- public static final ConfigEntry<Long> ROLE_CACHE_EVICTION_INTERVAL_MS =
- new ConfigBuilder("gravitino.authorization.roleCacheEvictionIntervalMs")
- .doc("The interval in milliseconds to evict the role cache")
- .version(ConfigConstants.VERSION_0_5_0)
- .longConf()
- .createWithDefault(60 * 60 * 1000L);
-
public static final int DEFAULT_METRICS_TIME_SLIDING_WINDOW_SECONDS = 60;
public static final ConfigEntry<Integer> METRICS_TIME_SLIDING_WINDOW_SECONDS
=
new ConfigBuilder("gravitino.metrics.timeSlidingWindowSecs")
diff --git
a/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java
b/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java
index 8c6a73346..aa890667d 100644
---
a/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java
+++
b/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java
@@ -45,7 +45,7 @@ public class AccessControlManager implements
AccessControlDispatcher {
private final List<String> serviceAdmins;
public AccessControlManager(EntityStore store, IdGenerator idGenerator,
Config config) {
- this.roleManager = new RoleManager(store, idGenerator, config);
+ this.roleManager = new RoleManager(store, idGenerator);
this.userGroupManager = new UserGroupManager(store, idGenerator);
this.permissionManager = new PermissionManager(store, roleManager);
this.serviceAdmins = config.get(Configs.SERVICE_ADMINS);
diff --git
a/core/src/main/java/org/apache/gravitino/authorization/RoleManager.java
b/core/src/main/java/org/apache/gravitino/authorization/RoleManager.java
index 3edcfaa57..457a7f5ff 100644
--- a/core/src/main/java/org/apache/gravitino/authorization/RoleManager.java
+++ b/core/src/main/java/org/apache/gravitino/authorization/RoleManager.java
@@ -19,19 +19,10 @@
package org.apache.gravitino.authorization;
-import com.github.benmanes.caffeine.cache.Cache;
-import com.github.benmanes.caffeine.cache.Caffeine;
-import com.github.benmanes.caffeine.cache.Scheduler;
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.util.concurrent.ThreadFactoryBuilder;
import java.io.IOException;
import java.time.Instant;
import java.util.List;
import java.util.Map;
-import java.util.concurrent.ScheduledThreadPoolExecutor;
-import java.util.concurrent.TimeUnit;
-import org.apache.gravitino.Config;
-import org.apache.gravitino.Configs;
import org.apache.gravitino.Entity;
import org.apache.gravitino.EntityAlreadyExistsException;
import org.apache.gravitino.EntityStore;
@@ -57,32 +48,10 @@ class RoleManager {
private static final Logger LOG = LoggerFactory.getLogger(RoleManager.class);
private final EntityStore store;
private final IdGenerator idGenerator;
- private final Cache<NameIdentifier, RoleEntity> cache;
- RoleManager(EntityStore store, IdGenerator idGenerator, Config config) {
+ RoleManager(EntityStore store, IdGenerator idGenerator) {
this.store = store;
this.idGenerator = idGenerator;
-
- long cacheEvictionIntervalInMs =
config.get(Configs.ROLE_CACHE_EVICTION_INTERVAL_MS);
- // One role entity is about 40 bytes using jol estimate, there are usually
about 100w+
- // roles in the production environment, this won't bring too much memory
cost, but it
- // can improve the performance significantly.
- this.cache =
- Caffeine.newBuilder()
- .expireAfterAccess(cacheEvictionIntervalInMs,
TimeUnit.MILLISECONDS)
- .removalListener(
- (k, v, c) -> {
- LOG.info("Remove role {} from the cache.", k);
- })
- .scheduler(
- Scheduler.forScheduledExecutorService(
- new ScheduledThreadPoolExecutor(
- 1,
- new ThreadFactoryBuilder()
- .setDaemon(true)
- .setNameFormat("role-cleaner-%d")
- .build())))
- .build();
}
RoleEntity createRole(
@@ -107,7 +76,6 @@ class RoleManager {
.build();
try {
store.put(roleEntity, false /* overwritten */);
- cache.put(roleEntity.nameIdentifier(), roleEntity);
AuthorizationUtils.callAuthorizationPluginForSecurableObjects(
metalake,
@@ -141,7 +109,6 @@ class RoleManager {
try {
AuthorizationUtils.checkMetalakeExists(metalake);
NameIdentifier ident = AuthorizationUtils.ofRole(metalake, role);
- cache.invalidate(ident);
try {
RoleEntity roleEntity = store.get(ident, Entity.EntityType.ROLE,
RoleEntity.class);
@@ -163,20 +130,11 @@ class RoleManager {
}
private RoleEntity getRoleEntity(NameIdentifier identifier) {
- return cache.get(
- identifier,
- id -> {
- try {
- return store.get(identifier, Entity.EntityType.ROLE,
RoleEntity.class);
- } catch (IOException ioe) {
- LOG.error("Failed to get roles {} due to storage issues",
identifier, ioe);
- throw new RuntimeException(ioe);
- }
- });
- }
-
- @VisibleForTesting
- Cache<NameIdentifier, RoleEntity> getCache() {
- return cache;
+ try {
+ return store.get(identifier, Entity.EntityType.ROLE, RoleEntity.class);
+ } catch (IOException ioe) {
+ LOG.error("Failed to get roles {} due to storage issues", identifier,
ioe);
+ throw new RuntimeException(ioe);
+ }
}
}
diff --git
a/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java
b/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java
index 27e5e667c..fd27771a0 100644
---
a/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java
+++
b/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java
@@ -268,13 +268,8 @@ public class TestAccessControlManager {
SecurableObjects.ofCatalog(
"catalog",
Lists.newArrayList(Privileges.UseCatalog.allow()))));
- Role cachedRole = accessControlManager.getRole("metalake", "loadRole");
- accessControlManager.getRoleManager().getCache().invalidateAll();
Role role = accessControlManager.getRole("metalake", "loadRole");
- // Verify the cached roleEntity is correct
- Assertions.assertEquals(role, cachedRole);
-
Assertions.assertEquals("loadRole", role.name());
testProperties(props, role.properties());
