Copilot commented on code in PR #11174:
URL: https://github.com/apache/gravitino/pull/11174#discussion_r3273833348
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -490,86 +614,244 @@ private boolean authorizeByJcasbin(
}
}
- private static UserEntity getUserEntity(String username, String metalake)
throws IOException {
+ //
---------------------------------------------------------------------------
+ // User info / ownership helpers
+ //
---------------------------------------------------------------------------
+
+ /**
+ * Per-request {@link UserUpdatedAt} lookup. The underlying {@code
user_meta} query is issued at
+ * most once per (metalake, username) within a single request.
+ */
+ private Optional<UserUpdatedAt> loadUserInfo(
+ String metalake, String username, AuthorizationRequestContext
requestContext) {
+ String cacheKey = metalake + KEY_SEP + username;
+ return requestContext.computeUserInfoIfAbsent(
+ cacheKey,
+ k ->
+ Optional.ofNullable(
+ SessionUtils.getWithoutCommit(
+ UserMetaMapper.class, m -> m.getUserUpdatedAt(metalake,
username))));
+ }
+
+ /**
+ * Batched version probe that collapses {@link #loadUserInfo} and a
per-group {@link
+ * #loadGroupInfo} fan-out into a single UNION query. After this returns,
both the per-request
+ * {@code userInfoCache} and {@code groupInfoCache} on {@code
requestContext} are primed: every
+ * requested group key is present (mapped to either a {@link GroupUpdatedAt}
when the row exists
+ * or {@link Optional#empty()} when the IdP-pushed name is stale).
+ *
+ * <p>Saves {@code N} round trips on the hot path: where the legacy flow
paid {@code 1 user_meta +
+ * N group_meta} probes, this pays {@code 1} UNION. {@code N=0} (principal
has no groups) degrades
+ * to a plain user-only SELECT via the SQL provider.
+ *
+ * <p>{@code computeXxxIfAbsent} is used to populate the caches so that an
entry already cached
+ * from a prior call in the same request is not overwritten — matching
{@link #loadUserInfo}'s and
+ * {@link #loadGroupInfo}'s "first wins" dedup semantics.
+ */
+ private Optional<UserUpdatedAt> prefetchUserAndGroupInfo(
+ String metalake,
+ String username,
+ List<String> groupNames,
+ AuthorizationRequestContext requestContext) {
+
+ List<AuthSubjectVersion> rows =
+ SessionUtils.getWithoutCommit(
+ UserMetaMapper.class,
+ m -> m.batchGetUserAndGroupUpdatedAt(metalake, username,
groupNames));
+
+ UserUpdatedAt foundUser = null;
+ Map<String, GroupUpdatedAt> foundGroups = new HashMap<>();
+ for (AuthSubjectVersion row : rows) {
+ if (SUBJECT_TYPE_USER.equals(row.getSubjectType())) {
+ foundUser = new UserUpdatedAt(row.getId(), row.getUpdatedAt());
+ } else if (SUBJECT_TYPE_GROUP.equals(row.getSubjectType())) {
+ foundGroups.put(row.getName(), new GroupUpdatedAt(row.getId(),
row.getUpdatedAt()));
+ }
+ }
+
+ String userKey = metalake + KEY_SEP + username;
+ final Optional<UserUpdatedAt> userValue = Optional.ofNullable(foundUser);
+ Optional<UserUpdatedAt> userOpt =
+ requestContext.computeUserInfoIfAbsent(userKey, k -> userValue);
+
+ // Negative-cache absent groups too so loadGroupRoles can short-circuit
without re-querying.
+ for (String groupName : groupNames) {
+ String groupKey = metalake + KEY_SEP + groupName;
+ final Optional<GroupUpdatedAt> groupValue =
Optional.ofNullable(foundGroups.get(groupName));
+ requestContext.computeGroupInfoIfAbsent(groupKey, k -> groupValue);
+ }
+
+ return userOpt;
+ }
+
+ /**
+ * Returns true when the cached owner type and ID match the current user or
one of the user's
+ * groups.
+ */
+ private boolean ownerMatchesUserOrGroups(
+ Optional<OwnerInfo> owner, long userId, String metalake) {
+ if (!owner.isPresent()) {
+ return false;
+ }
+ OwnerInfo ownerInfo = owner.get();
+ if
(Entity.EntityType.USER.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
+ return ownerInfo.getOwnerId() == userId;
+ }
+ if
(!Entity.EntityType.GROUP.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
+ return false;
+ }
EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- UserEntity userEntity =
- entityStore.get(
- NameIdentifierUtil.ofUser(metalake, username),
- Entity.EntityType.USER,
- UserEntity.class);
- return userEntity;
+ for (GroupEntity groupEntity : resolveCurrentUserGroups(metalake,
entityStore)) {
+ if (Objects.equals(groupEntity.id(), ownerInfo.getOwnerId())) {
+ return true;
+ }
+ }
+ return false;
}
+ //
---------------------------------------------------------------------------
+ // 4-step role loading with version validation
+ //
---------------------------------------------------------------------------
+
private void loadRolePrivilege(
- String metalake, String username, Long userId,
AuthorizationRequestContext requestContext) {
+ String metalake,
+ String username,
+ long userId,
+ UserUpdatedAt userInfo,
+ AuthorizationRequestContext requestContext) {
requestContext.loadRole(
() -> {
- EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- NameIdentifier userNameIdentifier =
NameIdentifierUtil.ofUser(metalake, username);
- List<RoleEntity> entities;
- try {
- entities =
- entityStore
- .relationOperations()
- .listEntitiesByRelation(
- SupportsRelationOperations.Type.ROLE_USER_REL,
- userNameIdentifier,
- Entity.EntityType.USER);
- List<CompletableFuture<Void>> loadRoleFutures = new ArrayList<>();
- Set<String> desiredRoleIds = new HashSet<>();
- for (RoleEntity role : entities) {
- desiredRoleIds.add(String.valueOf(role.id()));
- addRoleForUserAndLoadPolicies(
- userId, metalake, role.id(), role.name(), loadRoleFutures,
entityStore);
- }
+ // Step 1a: version-validated user-direct roles via cache.
+ List<Long> userDirectRoleIds = loadUserRoles(metalake, username,
userId, userInfo);
+
+ // Step 1b: version-validated group-inherited roles via cache. Group
membership comes
+ // from the IdP-pushed UserPrincipal; for each group we load its
roles via the same
+ // version-validated path as users (group_meta.updated_at as the
staleness sentinel).
+ List<Long> groupInheritedRoleIds = new ArrayList<>();
+ for (String groupname : currentPrincipalGroupNames()) {
+ groupInheritedRoleIds.addAll(
+ loadGroupRoles(metalake, groupname, userId, requestContext));
+ }
- // Load roles inherited from the user's groups.
- for (GroupEntity groupEntity : resolveCurrentUserGroups(metalake,
entityStore)) {
- List<Long> roleIds = groupEntity.roleIds();
- List<String> roleNames = groupEntity.roleNames();
- if (roleIds == null || roleNames == null) {
- continue;
- }
- if (roleIds.size() != roleNames.size()) {
- LOG.warn(
- "Group {} has mismatched roleIds ({}) and roleNames ({})
-- skipping",
- groupEntity.name(),
- roleIds.size(),
- roleNames.size());
- continue;
- }
- for (int i = 0; i < roleIds.size(); i++) {
- desiredRoleIds.add(String.valueOf(roleIds.get(i)));
- addRoleForUserAndLoadPolicies(
- userId,
- metalake,
- roleIds.get(i),
- roleNames.get(i),
- loadRoleFutures,
- entityStore);
- }
+ // Prune stale g-rows: any role currently bound but no longer in the
desired
+ // set (e.g. user removed from a group at the IdP, or role
unassigned).
+ Set<String> desiredRoleIds = new HashSet<>();
+ for (Long id : userDirectRoleIds) {
+ desiredRoleIds.add(String.valueOf(id));
+ }
+ for (Long id : groupInheritedRoleIds) {
+ desiredRoleIds.add(String.valueOf(id));
+ }
+ String userIdStr = String.valueOf(userId);
+ for (String currentRole : allowEnforcer.getRolesForUser(userIdStr)) {
+ if (!desiredRoleIds.contains(currentRole)) {
+ allowEnforcer.deleteRoleForUser(userIdStr, currentRole);
+ denyEnforcer.deleteRoleForUser(userIdStr, currentRole);
}
+ }
- CompletableFuture.allOf(loadRoleFutures.toArray(new
CompletableFuture[0])).join();
-
- // Prune stale g-rows: remove role mappings that are no longer
valid
- // (e.g. user was removed from a group at the IdP level).
- String userIdStr = String.valueOf(userId);
- for (String currentRole :
allowEnforcer.getRolesForUser(userIdStr)) {
- if (!desiredRoleIds.contains(currentRole)) {
- allowEnforcer.deleteRoleForUser(userIdStr, currentRole);
- denyEnforcer.deleteRoleForUser(userIdStr, currentRole);
- }
- }
- } catch (IOException e) {
- throw new RuntimeException(e);
+ // Step 3: batch version-check all role IDs (direct +
group-inherited),
+ // load stale ones (1 query for the version probe).
+ List<Long> allRoleIds = new ArrayList<>(userDirectRoleIds);
+ allRoleIds.addAll(groupInheritedRoleIds);
+ if (!allRoleIds.isEmpty()) {
+ versionCheckAndLoadRoles(metalake, allRoleIds, requestContext);
}
});
}
+ private List<Long> loadUserRoles(
+ String metalake, String username, long userId, UserUpdatedAt userInfo) {
+ String userCacheKey = metalake + KEY_SEP + username;
+ Optional<CachedUserRoles> cachedOpt =
userRoleCache.getIfPresent(userCacheKey);
+
+ if (cachedOpt.isPresent() && cachedOpt.get().getUpdatedAt() >=
userInfo.getUpdatedAt()) {
+ // Cache is still valid
+ CachedUserRoles cached = cachedOpt.get();
+ bindUserRoles(userId, cached.getRoleIds());
+ return cached.getRoleIds();
+ }
+
+ // Cache miss or stale — reload from DB
+ List<RolePO> rolePOs =
+ SessionUtils.getWithoutCommit(RoleMetaMapper.class, m ->
m.listRolesByUserId(userId));
+ List<Long> roleIds =
rolePOs.stream().map(RolePO::getRoleId).collect(Collectors.toList());
+
+ userRoleCache.put(userCacheKey, new CachedUserRoles(userId,
userInfo.getUpdatedAt(), roleIds));
+ bindUserRoles(userId, roleIds);
+ return roleIds;
+ }
+
+ /**
+ * Per-request {@link GroupUpdatedAt} lookup, mirroring {@link
#loadUserInfo}. The {@code
+ * group_meta} probe runs at most once per (metalake, groupname) within a
single request.
+ */
+ private Optional<GroupUpdatedAt> loadGroupInfo(
+ String metalake, String groupname, AuthorizationRequestContext
requestContext) {
+ String cacheKey = metalake + KEY_SEP + groupname;
+ return requestContext.computeGroupInfoIfAbsent(
+ cacheKey,
+ k ->
+ Optional.ofNullable(
+ SessionUtils.getWithoutCommit(
+ GroupMetaMapper.class, m -> m.getGroupUpdatedAt(metalake,
groupname))));
+ }
+
+ /**
+ * Version-validated group-role load, mirroring {@link #loadUserRoles}. Uses
{@code
+ * group_meta.updated_at} as the staleness sentinel: if the cached snapshot
is at least as fresh
+ * as the DB version, we reuse it; otherwise we reload from {@code
role_meta}. In both cases the
+ * resulting role IDs are bound to the user's jcasbin g-rows so that the
enforcer sees inherited
+ * privileges. Groups missing from the DB return an empty list.
+ */
+ private List<Long> loadGroupRoles(
+ String metalake, String groupname, long userId,
AuthorizationRequestContext requestContext) {
+ Optional<GroupUpdatedAt> groupInfoOpt = loadGroupInfo(metalake, groupname,
requestContext);
+ if (!groupInfoOpt.isPresent()) {
+ return new ArrayList<>();
+ }
+ GroupUpdatedAt groupInfo = groupInfoOpt.get();
+ long groupId = groupInfo.getGroupId();
+ String groupCacheKey = metalake + KEY_SEP + groupname;
+ Optional<CachedGroupRoles> cachedOpt =
groupRoleCache.getIfPresent(groupCacheKey);
+
+ if (cachedOpt.isPresent() && cachedOpt.get().getUpdatedAt() >=
groupInfo.getUpdatedAt()) {
+ CachedGroupRoles cached = cachedOpt.get();
+ bindUserRoles(userId, cached.getRoleIds());
+ return cached.getRoleIds();
+ }
+
+ List<RolePO> rolePOs =
+ SessionUtils.getWithoutCommit(RoleMetaMapper.class, m ->
m.listRolesByGroupId(groupId));
+ List<Long> roleIds =
rolePOs.stream().map(RolePO::getRoleId).collect(Collectors.toList());
+
+ groupRoleCache.put(
+ groupCacheKey, new CachedGroupRoles(groupId, groupInfo.getUpdatedAt(),
roleIds));
+ bindUserRoles(userId, roleIds);
+ return roleIds;
+ }
+
+ /**
+ * Returns the current principal's group names as carried by the IdP-pushed
{@link UserPrincipal}.
+ * Returns an empty list when the principal is not a {@link UserPrincipal}
(e.g. service tokens)
+ * or has no groups.
+ */
+ private List<String> currentPrincipalGroupNames() {
+ Principal principal = PrincipalUtils.getCurrentPrincipal();
+ if (!(principal instanceof UserPrincipal)) {
+ return new ArrayList<>();
+ }
+ List<UserGroup> groups = ((UserPrincipal) principal).getGroups();
+ if (groups.isEmpty()) {
+ return new ArrayList<>();
+ }
+ return
groups.stream().map(UserGroup::getGroupname).collect(Collectors.toList());
+ }
+
/**
* Resolves GroupEntity objects for the current principal's groups, skipping
any that are stale or
- * not found in the store.
+ * not found in the store. Used by both {@link #isSelf} (ROLE branch) and
{@link
+ * #loadRolePrivilege} to discover group-inherited role assignments.
*/
Review Comment:
This Javadoc is stale: resolveCurrentUserGroups() is no longer used by
isSelf(ROLE) or loadRolePrivilege (both now use currentPrincipalGroupNames +
mapper-based group lookups). Please update the comment to reflect current call
sites (e.g., ownership checks) to avoid misleading future changes.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinLoadedRolesCache.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.server.authorization.jcasbin;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import java.util.Optional;
+import java.util.concurrent.TimeUnit;
+import org.apache.gravitino.cache.GravitinoCache;
+import org.casbin.jcasbin.main.Enforcer;
+
+/**
+ * A {@link GravitinoCache} of {@code roleId -> updated_at} that synchronously
deletes the role's
+ * JCasbin policies from both enforcers when a key is evicted (by TTL, size,
or explicit
+ * invalidate).
+ *
+ * <p>Uses a raw Caffeine cache internally so it can attach a removal listener
with {@code
+ * executor(Runnable::run)} — eviction and policy cleanup must happen on the
same thread, so the
+ * {@link JcasbinAuthorizer} never sees a role bound in the enforcer without a
backing policy.
+ */
+class JcasbinLoadedRolesCache implements GravitinoCache<Long, Long> {
+
+ private final Cache<Long, Long> cache;
+
+ JcasbinLoadedRolesCache(long ttlMs, long maxSize, Enforcer allowEnforcer,
Enforcer denyEnforcer) {
+ this.cache =
+ Caffeine.newBuilder()
+ .expireAfterAccess(ttlMs, TimeUnit.MILLISECONDS)
+ .maximumSize(maxSize)
+ .executor(Runnable::run)
+ .removalListener(
+ (roleId, value, cause) -> {
+ if (roleId != null) {
+ allowEnforcer.deleteRole(String.valueOf(roleId));
+ denyEnforcer.deleteRole(String.valueOf(roleId));
+ }
+ })
+ .build();
+ }
+
+ @Override
+ public Optional<Long> getIfPresent(Long key) {
+ return Optional.ofNullable(cache.getIfPresent(key));
+ }
+
+ @Override
+ public void put(Long key, Long value) {
+ cache.put(key, value);
+ }
+
+ @Override
+ public void invalidate(Long key) {
+ cache.invalidate(key);
+ }
+
+ @Override
+ public void invalidateAll() {
+ cache.invalidateAll();
+ }
+
+ @Override
+ public void invalidateByPrefix(String prefix) {
+ cache.asMap().keySet().removeIf(k -> k.toString().startsWith(prefix));
Review Comment:
invalidateByPrefix() is dangerous here: keys are Long, so prefix matching
via toString() can evict unintended entries (e.g., prefix "1" removes 1, 10,
100, ...). Since this cache isn't string-keyed, prefer a no-op implementation
(or throw UnsupportedOperationException) to avoid accidental broad evictions.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -490,86 +614,244 @@ private boolean authorizeByJcasbin(
}
}
- private static UserEntity getUserEntity(String username, String metalake)
throws IOException {
+ //
---------------------------------------------------------------------------
+ // User info / ownership helpers
+ //
---------------------------------------------------------------------------
+
+ /**
+ * Per-request {@link UserUpdatedAt} lookup. The underlying {@code
user_meta} query is issued at
+ * most once per (metalake, username) within a single request.
+ */
+ private Optional<UserUpdatedAt> loadUserInfo(
+ String metalake, String username, AuthorizationRequestContext
requestContext) {
+ String cacheKey = metalake + KEY_SEP + username;
+ return requestContext.computeUserInfoIfAbsent(
+ cacheKey,
+ k ->
+ Optional.ofNullable(
+ SessionUtils.getWithoutCommit(
+ UserMetaMapper.class, m -> m.getUserUpdatedAt(metalake,
username))));
+ }
+
+ /**
+ * Batched version probe that collapses {@link #loadUserInfo} and a
per-group {@link
+ * #loadGroupInfo} fan-out into a single UNION query. After this returns,
both the per-request
+ * {@code userInfoCache} and {@code groupInfoCache} on {@code
requestContext} are primed: every
+ * requested group key is present (mapped to either a {@link GroupUpdatedAt}
when the row exists
+ * or {@link Optional#empty()} when the IdP-pushed name is stale).
+ *
+ * <p>Saves {@code N} round trips on the hot path: where the legacy flow
paid {@code 1 user_meta +
+ * N group_meta} probes, this pays {@code 1} UNION. {@code N=0} (principal
has no groups) degrades
+ * to a plain user-only SELECT via the SQL provider.
+ *
+ * <p>{@code computeXxxIfAbsent} is used to populate the caches so that an
entry already cached
+ * from a prior call in the same request is not overwritten — matching
{@link #loadUserInfo}'s and
+ * {@link #loadGroupInfo}'s "first wins" dedup semantics.
+ */
+ private Optional<UserUpdatedAt> prefetchUserAndGroupInfo(
+ String metalake,
+ String username,
+ List<String> groupNames,
+ AuthorizationRequestContext requestContext) {
+
+ List<AuthSubjectVersion> rows =
+ SessionUtils.getWithoutCommit(
+ UserMetaMapper.class,
+ m -> m.batchGetUserAndGroupUpdatedAt(metalake, username,
groupNames));
+
+ UserUpdatedAt foundUser = null;
+ Map<String, GroupUpdatedAt> foundGroups = new HashMap<>();
+ for (AuthSubjectVersion row : rows) {
+ if (SUBJECT_TYPE_USER.equals(row.getSubjectType())) {
+ foundUser = new UserUpdatedAt(row.getId(), row.getUpdatedAt());
+ } else if (SUBJECT_TYPE_GROUP.equals(row.getSubjectType())) {
+ foundGroups.put(row.getName(), new GroupUpdatedAt(row.getId(),
row.getUpdatedAt()));
+ }
+ }
+
+ String userKey = metalake + KEY_SEP + username;
+ final Optional<UserUpdatedAt> userValue = Optional.ofNullable(foundUser);
+ Optional<UserUpdatedAt> userOpt =
+ requestContext.computeUserInfoIfAbsent(userKey, k -> userValue);
+
+ // Negative-cache absent groups too so loadGroupRoles can short-circuit
without re-querying.
+ for (String groupName : groupNames) {
+ String groupKey = metalake + KEY_SEP + groupName;
+ final Optional<GroupUpdatedAt> groupValue =
Optional.ofNullable(foundGroups.get(groupName));
+ requestContext.computeGroupInfoIfAbsent(groupKey, k -> groupValue);
+ }
+
+ return userOpt;
+ }
+
+ /**
+ * Returns true when the cached owner type and ID match the current user or
one of the user's
+ * groups.
+ */
+ private boolean ownerMatchesUserOrGroups(
+ Optional<OwnerInfo> owner, long userId, String metalake) {
+ if (!owner.isPresent()) {
+ return false;
+ }
+ OwnerInfo ownerInfo = owner.get();
+ if
(Entity.EntityType.USER.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
+ return ownerInfo.getOwnerId() == userId;
+ }
+ if
(!Entity.EntityType.GROUP.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
+ return false;
+ }
EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- UserEntity userEntity =
- entityStore.get(
- NameIdentifierUtil.ofUser(metalake, username),
- Entity.EntityType.USER,
- UserEntity.class);
- return userEntity;
+ for (GroupEntity groupEntity : resolveCurrentUserGroups(metalake,
entityStore)) {
+ if (Objects.equals(groupEntity.id(), ownerInfo.getOwnerId())) {
+ return true;
+ }
+ }
+ return false;
}
+ //
---------------------------------------------------------------------------
+ // 4-step role loading with version validation
+ //
---------------------------------------------------------------------------
+
private void loadRolePrivilege(
- String metalake, String username, Long userId,
AuthorizationRequestContext requestContext) {
+ String metalake,
+ String username,
+ long userId,
+ UserUpdatedAt userInfo,
+ AuthorizationRequestContext requestContext) {
requestContext.loadRole(
() -> {
- EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- NameIdentifier userNameIdentifier =
NameIdentifierUtil.ofUser(metalake, username);
- List<RoleEntity> entities;
- try {
- entities =
- entityStore
- .relationOperations()
- .listEntitiesByRelation(
- SupportsRelationOperations.Type.ROLE_USER_REL,
- userNameIdentifier,
- Entity.EntityType.USER);
- List<CompletableFuture<Void>> loadRoleFutures = new ArrayList<>();
- Set<String> desiredRoleIds = new HashSet<>();
- for (RoleEntity role : entities) {
- desiredRoleIds.add(String.valueOf(role.id()));
- addRoleForUserAndLoadPolicies(
- userId, metalake, role.id(), role.name(), loadRoleFutures,
entityStore);
- }
+ // Step 1a: version-validated user-direct roles via cache.
+ List<Long> userDirectRoleIds = loadUserRoles(metalake, username,
userId, userInfo);
+
+ // Step 1b: version-validated group-inherited roles via cache. Group
membership comes
+ // from the IdP-pushed UserPrincipal; for each group we load its
roles via the same
+ // version-validated path as users (group_meta.updated_at as the
staleness sentinel).
+ List<Long> groupInheritedRoleIds = new ArrayList<>();
+ for (String groupname : currentPrincipalGroupNames()) {
+ groupInheritedRoleIds.addAll(
+ loadGroupRoles(metalake, groupname, userId, requestContext));
+ }
- // Load roles inherited from the user's groups.
- for (GroupEntity groupEntity : resolveCurrentUserGroups(metalake,
entityStore)) {
- List<Long> roleIds = groupEntity.roleIds();
- List<String> roleNames = groupEntity.roleNames();
- if (roleIds == null || roleNames == null) {
- continue;
- }
- if (roleIds.size() != roleNames.size()) {
- LOG.warn(
- "Group {} has mismatched roleIds ({}) and roleNames ({})
-- skipping",
- groupEntity.name(),
- roleIds.size(),
- roleNames.size());
- continue;
- }
- for (int i = 0; i < roleIds.size(); i++) {
- desiredRoleIds.add(String.valueOf(roleIds.get(i)));
- addRoleForUserAndLoadPolicies(
- userId,
- metalake,
- roleIds.get(i),
- roleNames.get(i),
- loadRoleFutures,
- entityStore);
- }
+ // Prune stale g-rows: any role currently bound but no longer in the
desired
+ // set (e.g. user removed from a group at the IdP, or role
unassigned).
+ Set<String> desiredRoleIds = new HashSet<>();
+ for (Long id : userDirectRoleIds) {
+ desiredRoleIds.add(String.valueOf(id));
+ }
+ for (Long id : groupInheritedRoleIds) {
+ desiredRoleIds.add(String.valueOf(id));
+ }
+ String userIdStr = String.valueOf(userId);
+ for (String currentRole : allowEnforcer.getRolesForUser(userIdStr)) {
+ if (!desiredRoleIds.contains(currentRole)) {
+ allowEnforcer.deleteRoleForUser(userIdStr, currentRole);
+ denyEnforcer.deleteRoleForUser(userIdStr, currentRole);
}
+ }
- CompletableFuture.allOf(loadRoleFutures.toArray(new
CompletableFuture[0])).join();
-
- // Prune stale g-rows: remove role mappings that are no longer
valid
- // (e.g. user was removed from a group at the IdP level).
- String userIdStr = String.valueOf(userId);
- for (String currentRole :
allowEnforcer.getRolesForUser(userIdStr)) {
- if (!desiredRoleIds.contains(currentRole)) {
- allowEnforcer.deleteRoleForUser(userIdStr, currentRole);
- denyEnforcer.deleteRoleForUser(userIdStr, currentRole);
- }
- }
- } catch (IOException e) {
- throw new RuntimeException(e);
+ // Step 3: batch version-check all role IDs (direct +
group-inherited),
+ // load stale ones (1 query for the version probe).
+ List<Long> allRoleIds = new ArrayList<>(userDirectRoleIds);
+ allRoleIds.addAll(groupInheritedRoleIds);
+ if (!allRoleIds.isEmpty()) {
+ versionCheckAndLoadRoles(metalake, allRoleIds, requestContext);
}
});
}
+ private List<Long> loadUserRoles(
+ String metalake, String username, long userId, UserUpdatedAt userInfo) {
+ String userCacheKey = metalake + KEY_SEP + username;
+ Optional<CachedUserRoles> cachedOpt =
userRoleCache.getIfPresent(userCacheKey);
+
+ if (cachedOpt.isPresent() && cachedOpt.get().getUpdatedAt() >=
userInfo.getUpdatedAt()) {
+ // Cache is still valid
+ CachedUserRoles cached = cachedOpt.get();
+ bindUserRoles(userId, cached.getRoleIds());
+ return cached.getRoleIds();
+ }
+
+ // Cache miss or stale — reload from DB
+ List<RolePO> rolePOs =
+ SessionUtils.getWithoutCommit(RoleMetaMapper.class, m ->
m.listRolesByUserId(userId));
+ List<Long> roleIds =
rolePOs.stream().map(RolePO::getRoleId).collect(Collectors.toList());
+
+ userRoleCache.put(userCacheKey, new CachedUserRoles(userId,
userInfo.getUpdatedAt(), roleIds));
+ bindUserRoles(userId, roleIds);
+ return roleIds;
+ }
+
+ /**
+ * Per-request {@link GroupUpdatedAt} lookup, mirroring {@link
#loadUserInfo}. The {@code
+ * group_meta} probe runs at most once per (metalake, groupname) within a
single request.
+ */
+ private Optional<GroupUpdatedAt> loadGroupInfo(
+ String metalake, String groupname, AuthorizationRequestContext
requestContext) {
+ String cacheKey = metalake + KEY_SEP + groupname;
+ return requestContext.computeGroupInfoIfAbsent(
+ cacheKey,
+ k ->
+ Optional.ofNullable(
+ SessionUtils.getWithoutCommit(
+ GroupMetaMapper.class, m -> m.getGroupUpdatedAt(metalake,
groupname))));
+ }
+
+ /**
+ * Version-validated group-role load, mirroring {@link #loadUserRoles}. Uses
{@code
+ * group_meta.updated_at} as the staleness sentinel: if the cached snapshot
is at least as fresh
+ * as the DB version, we reuse it; otherwise we reload from {@code
role_meta}. In both cases the
+ * resulting role IDs are bound to the user's jcasbin g-rows so that the
enforcer sees inherited
+ * privileges. Groups missing from the DB return an empty list.
+ */
+ private List<Long> loadGroupRoles(
+ String metalake, String groupname, long userId,
AuthorizationRequestContext requestContext) {
+ Optional<GroupUpdatedAt> groupInfoOpt = loadGroupInfo(metalake, groupname,
requestContext);
+ if (!groupInfoOpt.isPresent()) {
+ return new ArrayList<>();
+ }
+ GroupUpdatedAt groupInfo = groupInfoOpt.get();
+ long groupId = groupInfo.getGroupId();
+ String groupCacheKey = metalake + KEY_SEP + groupname;
+ Optional<CachedGroupRoles> cachedOpt =
groupRoleCache.getIfPresent(groupCacheKey);
+
+ if (cachedOpt.isPresent() && cachedOpt.get().getUpdatedAt() >=
groupInfo.getUpdatedAt()) {
+ CachedGroupRoles cached = cachedOpt.get();
+ bindUserRoles(userId, cached.getRoleIds());
+ return cached.getRoleIds();
Review Comment:
The groupRoleCache validity check only compares updatedAt. Because
group_meta.updated_at defaults to 0 on insert, deleting/recreating a group with
the same name can make an older CachedGroupRoles entry appear "fresh" and apply
stale roleIds to the new groupId. Require cached.groupId == groupId (and
consider keying by groupId) before reusing cached role snapshots.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -490,86 +614,244 @@ private boolean authorizeByJcasbin(
}
}
- private static UserEntity getUserEntity(String username, String metalake)
throws IOException {
+ //
---------------------------------------------------------------------------
+ // User info / ownership helpers
+ //
---------------------------------------------------------------------------
+
+ /**
+ * Per-request {@link UserUpdatedAt} lookup. The underlying {@code
user_meta} query is issued at
+ * most once per (metalake, username) within a single request.
+ */
+ private Optional<UserUpdatedAt> loadUserInfo(
+ String metalake, String username, AuthorizationRequestContext
requestContext) {
+ String cacheKey = metalake + KEY_SEP + username;
+ return requestContext.computeUserInfoIfAbsent(
+ cacheKey,
+ k ->
+ Optional.ofNullable(
+ SessionUtils.getWithoutCommit(
+ UserMetaMapper.class, m -> m.getUserUpdatedAt(metalake,
username))));
+ }
+
+ /**
+ * Batched version probe that collapses {@link #loadUserInfo} and a
per-group {@link
+ * #loadGroupInfo} fan-out into a single UNION query. After this returns,
both the per-request
+ * {@code userInfoCache} and {@code groupInfoCache} on {@code
requestContext} are primed: every
+ * requested group key is present (mapped to either a {@link GroupUpdatedAt}
when the row exists
+ * or {@link Optional#empty()} when the IdP-pushed name is stale).
+ *
+ * <p>Saves {@code N} round trips on the hot path: where the legacy flow
paid {@code 1 user_meta +
+ * N group_meta} probes, this pays {@code 1} UNION. {@code N=0} (principal
has no groups) degrades
+ * to a plain user-only SELECT via the SQL provider.
+ *
+ * <p>{@code computeXxxIfAbsent} is used to populate the caches so that an
entry already cached
+ * from a prior call in the same request is not overwritten — matching
{@link #loadUserInfo}'s and
+ * {@link #loadGroupInfo}'s "first wins" dedup semantics.
+ */
+ private Optional<UserUpdatedAt> prefetchUserAndGroupInfo(
+ String metalake,
+ String username,
+ List<String> groupNames,
+ AuthorizationRequestContext requestContext) {
+
+ List<AuthSubjectVersion> rows =
+ SessionUtils.getWithoutCommit(
+ UserMetaMapper.class,
+ m -> m.batchGetUserAndGroupUpdatedAt(metalake, username,
groupNames));
+
+ UserUpdatedAt foundUser = null;
+ Map<String, GroupUpdatedAt> foundGroups = new HashMap<>();
+ for (AuthSubjectVersion row : rows) {
+ if (SUBJECT_TYPE_USER.equals(row.getSubjectType())) {
+ foundUser = new UserUpdatedAt(row.getId(), row.getUpdatedAt());
+ } else if (SUBJECT_TYPE_GROUP.equals(row.getSubjectType())) {
+ foundGroups.put(row.getName(), new GroupUpdatedAt(row.getId(),
row.getUpdatedAt()));
+ }
+ }
+
+ String userKey = metalake + KEY_SEP + username;
+ final Optional<UserUpdatedAt> userValue = Optional.ofNullable(foundUser);
+ Optional<UserUpdatedAt> userOpt =
+ requestContext.computeUserInfoIfAbsent(userKey, k -> userValue);
+
+ // Negative-cache absent groups too so loadGroupRoles can short-circuit
without re-querying.
+ for (String groupName : groupNames) {
+ String groupKey = metalake + KEY_SEP + groupName;
+ final Optional<GroupUpdatedAt> groupValue =
Optional.ofNullable(foundGroups.get(groupName));
+ requestContext.computeGroupInfoIfAbsent(groupKey, k -> groupValue);
+ }
+
+ return userOpt;
+ }
+
+ /**
+ * Returns true when the cached owner type and ID match the current user or
one of the user's
+ * groups.
+ */
+ private boolean ownerMatchesUserOrGroups(
+ Optional<OwnerInfo> owner, long userId, String metalake) {
+ if (!owner.isPresent()) {
+ return false;
+ }
+ OwnerInfo ownerInfo = owner.get();
+ if
(Entity.EntityType.USER.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
+ return ownerInfo.getOwnerId() == userId;
+ }
+ if
(!Entity.EntityType.GROUP.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
+ return false;
+ }
EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- UserEntity userEntity =
- entityStore.get(
- NameIdentifierUtil.ofUser(metalake, username),
- Entity.EntityType.USER,
- UserEntity.class);
- return userEntity;
+ for (GroupEntity groupEntity : resolveCurrentUserGroups(metalake,
entityStore)) {
+ if (Objects.equals(groupEntity.id(), ownerInfo.getOwnerId())) {
+ return true;
+ }
+ }
+ return false;
}
+ //
---------------------------------------------------------------------------
+ // 4-step role loading with version validation
+ //
---------------------------------------------------------------------------
+
private void loadRolePrivilege(
- String metalake, String username, Long userId,
AuthorizationRequestContext requestContext) {
+ String metalake,
+ String username,
+ long userId,
+ UserUpdatedAt userInfo,
+ AuthorizationRequestContext requestContext) {
requestContext.loadRole(
() -> {
- EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- NameIdentifier userNameIdentifier =
NameIdentifierUtil.ofUser(metalake, username);
- List<RoleEntity> entities;
- try {
- entities =
- entityStore
- .relationOperations()
- .listEntitiesByRelation(
- SupportsRelationOperations.Type.ROLE_USER_REL,
- userNameIdentifier,
- Entity.EntityType.USER);
- List<CompletableFuture<Void>> loadRoleFutures = new ArrayList<>();
- Set<String> desiredRoleIds = new HashSet<>();
- for (RoleEntity role : entities) {
- desiredRoleIds.add(String.valueOf(role.id()));
- addRoleForUserAndLoadPolicies(
- userId, metalake, role.id(), role.name(), loadRoleFutures,
entityStore);
- }
+ // Step 1a: version-validated user-direct roles via cache.
+ List<Long> userDirectRoleIds = loadUserRoles(metalake, username,
userId, userInfo);
+
+ // Step 1b: version-validated group-inherited roles via cache. Group
membership comes
+ // from the IdP-pushed UserPrincipal; for each group we load its
roles via the same
+ // version-validated path as users (group_meta.updated_at as the
staleness sentinel).
+ List<Long> groupInheritedRoleIds = new ArrayList<>();
+ for (String groupname : currentPrincipalGroupNames()) {
+ groupInheritedRoleIds.addAll(
+ loadGroupRoles(metalake, groupname, userId, requestContext));
+ }
- // Load roles inherited from the user's groups.
- for (GroupEntity groupEntity : resolveCurrentUserGroups(metalake,
entityStore)) {
- List<Long> roleIds = groupEntity.roleIds();
- List<String> roleNames = groupEntity.roleNames();
- if (roleIds == null || roleNames == null) {
- continue;
- }
- if (roleIds.size() != roleNames.size()) {
- LOG.warn(
- "Group {} has mismatched roleIds ({}) and roleNames ({})
-- skipping",
- groupEntity.name(),
- roleIds.size(),
- roleNames.size());
- continue;
- }
- for (int i = 0; i < roleIds.size(); i++) {
- desiredRoleIds.add(String.valueOf(roleIds.get(i)));
- addRoleForUserAndLoadPolicies(
- userId,
- metalake,
- roleIds.get(i),
- roleNames.get(i),
- loadRoleFutures,
- entityStore);
- }
+ // Prune stale g-rows: any role currently bound but no longer in the
desired
+ // set (e.g. user removed from a group at the IdP, or role
unassigned).
+ Set<String> desiredRoleIds = new HashSet<>();
+ for (Long id : userDirectRoleIds) {
+ desiredRoleIds.add(String.valueOf(id));
+ }
+ for (Long id : groupInheritedRoleIds) {
+ desiredRoleIds.add(String.valueOf(id));
+ }
+ String userIdStr = String.valueOf(userId);
+ for (String currentRole : allowEnforcer.getRolesForUser(userIdStr)) {
+ if (!desiredRoleIds.contains(currentRole)) {
+ allowEnforcer.deleteRoleForUser(userIdStr, currentRole);
+ denyEnforcer.deleteRoleForUser(userIdStr, currentRole);
}
+ }
- CompletableFuture.allOf(loadRoleFutures.toArray(new
CompletableFuture[0])).join();
-
- // Prune stale g-rows: remove role mappings that are no longer
valid
- // (e.g. user was removed from a group at the IdP level).
- String userIdStr = String.valueOf(userId);
- for (String currentRole :
allowEnforcer.getRolesForUser(userIdStr)) {
- if (!desiredRoleIds.contains(currentRole)) {
- allowEnforcer.deleteRoleForUser(userIdStr, currentRole);
- denyEnforcer.deleteRoleForUser(userIdStr, currentRole);
- }
- }
- } catch (IOException e) {
- throw new RuntimeException(e);
+ // Step 3: batch version-check all role IDs (direct +
group-inherited),
+ // load stale ones (1 query for the version probe).
+ List<Long> allRoleIds = new ArrayList<>(userDirectRoleIds);
+ allRoleIds.addAll(groupInheritedRoleIds);
+ if (!allRoleIds.isEmpty()) {
+ versionCheckAndLoadRoles(metalake, allRoleIds, requestContext);
}
});
}
+ private List<Long> loadUserRoles(
+ String metalake, String username, long userId, UserUpdatedAt userInfo) {
+ String userCacheKey = metalake + KEY_SEP + username;
+ Optional<CachedUserRoles> cachedOpt =
userRoleCache.getIfPresent(userCacheKey);
+
+ if (cachedOpt.isPresent() && cachedOpt.get().getUpdatedAt() >=
userInfo.getUpdatedAt()) {
+ // Cache is still valid
+ CachedUserRoles cached = cachedOpt.get();
+ bindUserRoles(userId, cached.getRoleIds());
+ return cached.getRoleIds();
Review Comment:
The cache validity check only compares updatedAt. Because
user_meta.updated_at defaults to 0 on insert, deleting/recreating a user with
the same name can produce a newer userId with a smaller updatedAt, causing an
old CachedUserRoles entry to be treated as valid and its roleIds to be bound to
the new userId (privilege escalation). Include a userId match in the validity
check (and/or key the cache by userId) before reusing a cached snapshot.
##########
core/src/test/java/org/apache/gravitino/cache/TestGravitinoCache.java:
##########
@@ -53,6 +59,148 @@ void testCaffeinePutAndGet() {
}
}
+ @Test
+ void testCaffeineGetLoadsSameKeyAtomically() throws Exception {
+ CaffeineGravitinoCache<String, Long> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ ExecutorService executorService = Executors.newFixedThreadPool(8);
+ try {
+ AtomicLong loadCount = new AtomicLong();
+ CountDownLatch ready = new CountDownLatch(8);
+ CountDownLatch start = new CountDownLatch(1);
+ List<Future<Long>> futures = new ArrayList<>();
+
+ for (int i = 0; i < 8; i++) {
+ futures.add(
+ executorService.submit(
+ () -> {
+ ready.countDown();
+ start.await();
+ return cache.get(
+ "shared",
+ key -> {
+ loadCount.incrementAndGet();
+ return 100L;
+ });
+ }));
+ }
+
+ Assertions.assertTrue(ready.await(5, TimeUnit.SECONDS));
+ start.countDown();
+ for (Future<Long> future : futures) {
+ Assertions.assertEquals(100L, future.get(5, TimeUnit.SECONDS));
+ }
+
+ Assertions.assertEquals(1L, loadCount.get());
+ Assertions.assertEquals(1L, cache.size());
+ } finally {
+ executorService.shutdownNow();
+ cache.close();
+ }
+ }
+
+ @Test
+ void testCaffeineInvalidateWaitsForInFlightReader() throws Exception {
+ CaffeineGravitinoCache<String, Long> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ ExecutorService executorService = Executors.newFixedThreadPool(2);
+ try {
+ CountDownLatch readerHoldsLock = new CountDownLatch(1);
+ CountDownLatch readerMayProceed = new CountDownLatch(1);
+
+ Future<Long> reader =
+ executorService.submit(
+ () ->
+ cache.get(
+ "k",
+ key -> {
+ readerHoldsLock.countDown();
+ try {
+ Assertions.assertTrue(readerMayProceed.await(5,
TimeUnit.SECONDS));
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException(e);
+ }
+ return 42L;
+ }));
+
+ Assertions.assertTrue(readerHoldsLock.await(5, TimeUnit.SECONDS));
+ Future<?> invalidator = executorService.submit(() ->
cache.invalidate("k"));
+
+ // Invalidator should be parked on the write lock until the reader
releases its read lock.
+ Thread.sleep(150);
+ Assertions.assertFalse(
+ invalidator.isDone(), "invalidate must not proceed while a reader
holds the read lock");
+
+ readerMayProceed.countDown();
+ Assertions.assertEquals(42L, reader.get(5, TimeUnit.SECONDS));
+ invalidator.get(5, TimeUnit.SECONDS);
+
+ // Reader installed the value, invalidator then removed it.
+ Assertions.assertFalse(cache.getIfPresent("k").isPresent());
+ } finally {
+ executorService.shutdownNow();
+ cache.close();
+ }
+ }
+
+ @Test
+ void testCaffeineRunInvalidationBatchIsAtomicForReaders() throws Exception {
+ CaffeineGravitinoCache<String, Long> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ ExecutorService executorService = Executors.newFixedThreadPool(2);
+ try {
+ cache.put("k1", 1L);
+ cache.put("k2", 2L);
+ cache.put("k3", 3L);
+
+ CountDownLatch batchEntered = new CountDownLatch(1);
+ CountDownLatch batchMayProceed = new CountDownLatch(1);
+
+ Future<?> batcher =
+ executorService.submit(
+ () ->
+ cache.runInvalidationBatch(
+ () -> {
+ batchEntered.countDown();
+ try {
+ Assertions.assertTrue(batchMayProceed.await(5,
TimeUnit.SECONDS));
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException(e);
+ }
+ cache.invalidate("k1");
+ cache.invalidate("k2");
+ cache.invalidate("k3");
+ }));
+
+ Assertions.assertTrue(batchEntered.await(5, TimeUnit.SECONDS));
+
+ // Reader started while the batcher holds the write lock must block
until the batch ends.
+ Future<Optional<Long>> reader = executorService.submit(() ->
cache.getIfPresent("k2"));
+ Thread.sleep(150);
+ Assertions.assertFalse(reader.isDone(), "reader must wait for the batch
to release the lock");
Review Comment:
This check uses Thread.sleep + Future.isDone() without guaranteeing the
reader task has reached getIfPresent() and is blocked on the lock, which can
make the test timing-dependent. Prefer a latch inside the reader (or Awaitility
with a "started" signal) to deterministically assert the reader is blocked
until the invalidation batch completes.
##########
core/src/test/java/org/apache/gravitino/cache/TestGravitinoCache.java:
##########
@@ -53,6 +59,148 @@ void testCaffeinePutAndGet() {
}
}
+ @Test
+ void testCaffeineGetLoadsSameKeyAtomically() throws Exception {
+ CaffeineGravitinoCache<String, Long> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ ExecutorService executorService = Executors.newFixedThreadPool(8);
+ try {
+ AtomicLong loadCount = new AtomicLong();
+ CountDownLatch ready = new CountDownLatch(8);
+ CountDownLatch start = new CountDownLatch(1);
+ List<Future<Long>> futures = new ArrayList<>();
+
+ for (int i = 0; i < 8; i++) {
+ futures.add(
+ executorService.submit(
+ () -> {
+ ready.countDown();
+ start.await();
+ return cache.get(
+ "shared",
+ key -> {
+ loadCount.incrementAndGet();
+ return 100L;
+ });
+ }));
+ }
+
+ Assertions.assertTrue(ready.await(5, TimeUnit.SECONDS));
+ start.countDown();
+ for (Future<Long> future : futures) {
+ Assertions.assertEquals(100L, future.get(5, TimeUnit.SECONDS));
+ }
+
+ Assertions.assertEquals(1L, loadCount.get());
+ Assertions.assertEquals(1L, cache.size());
+ } finally {
+ executorService.shutdownNow();
+ cache.close();
+ }
+ }
+
+ @Test
+ void testCaffeineInvalidateWaitsForInFlightReader() throws Exception {
+ CaffeineGravitinoCache<String, Long> cache = new
CaffeineGravitinoCache<>(60_000L, 1000L);
+ ExecutorService executorService = Executors.newFixedThreadPool(2);
+ try {
+ CountDownLatch readerHoldsLock = new CountDownLatch(1);
+ CountDownLatch readerMayProceed = new CountDownLatch(1);
+
+ Future<Long> reader =
+ executorService.submit(
+ () ->
+ cache.get(
+ "k",
+ key -> {
+ readerHoldsLock.countDown();
+ try {
+ Assertions.assertTrue(readerMayProceed.await(5,
TimeUnit.SECONDS));
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException(e);
+ }
+ return 42L;
+ }));
+
+ Assertions.assertTrue(readerHoldsLock.await(5, TimeUnit.SECONDS));
+ Future<?> invalidator = executorService.submit(() ->
cache.invalidate("k"));
+
+ // Invalidator should be parked on the write lock until the reader
releases its read lock.
+ Thread.sleep(150);
+ Assertions.assertFalse(
+ invalidator.isDone(), "invalidate must not proceed while a reader
holds the read lock");
+
Review Comment:
This concurrency assertion can be flaky because it relies on a fixed
Thread.sleep and doesn't ensure the invalidator task has actually started
attempting invalidate(). Consider adding a latch/flag inside the invalidator
runnable (or using Awaitility) so the test deterministically verifies that
invalidate is blocked by the read lock, not just unscheduled.
--
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]
