yuqi1129 commented on code in PR #14045:
URL: https://github.com/apache/gravitino/pull/14045#discussion_r4386086431
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -1416,72 +1436,192 @@ private Set<Long> versionCheckAndLoadRoles(
List<RoleUpdatedAt> staleRoleVersions = new ArrayList<>();
for (RoleUpdatedAt rv : roleVersions) {
- Optional<Long> cachedUpdatedAt =
loadedRoles.getIfPresent(rv.getRoleId());
- if (cachedUpdatedAt.isPresent() && cachedUpdatedAt.get() >=
rv.getUpdatedAt()) {
- continue;
+ if (needsRolePolicyLoad(rv)) {
+ staleRoleVersions.add(rv);
}
- // A role missing from loadedRoles because its last load was incomplete
would otherwise be
- // retried on every single request. Its partially loaded policies stay
in the enforcer, so
- // the privileges that did resolve keep working while the backoff is in
effect.
- if (!cachedUpdatedAt.isPresent()
- && partialRoleLoadBackoff.getIfPresent(rv.getRoleId()).isPresent()) {
- continue;
- }
- staleRoleVersions.add(rv);
}
if (staleRoleVersions.isEmpty()) {
return Collections.emptySet();
}
+ List<Long> leasedRoles = new ArrayList<>();
+ try {
+ return coalesceRolePolicyLoads(metalake, staleRoleVersions,
requestContext, leasedRoles);
+ } finally {
+ releaseRolePolicyLoadStates(leasedRoles);
+ }
+ }
+
+ private Set<Long> coalesceRolePolicyLoads(
+ String metalake,
+ List<RoleUpdatedAt> staleRoleVersions,
+ AuthorizationRequestContext requestContext,
+ List<Long> leasedRoles) {
+ Map<RolePolicyLoadKey, CompletableFuture<Boolean>> requestLoads = new
LinkedHashMap<>();
+ List<RolePolicyLoadKey> ownedLoads = new ArrayList<>();
+ try {
+ for (RoleUpdatedAt rv : staleRoleVersions) {
+ RolePolicyLoadKey key = new RolePolicyLoadKey(rv,
roleClearGeneration(rv.getRoleId()));
+ retainRolePolicyLoadState(key);
+ leasedRoles.add(key.roleVersion.getRoleId());
+ CompletableFuture<Boolean> result = new CompletableFuture<>();
+ CompletableFuture<Boolean> existing = rolePolicyLoads.putIfAbsent(key,
result);
+ requestLoads.put(key, existing == null ? result : existing);
+ if (existing == null) {
+ ownedLoads.add(key);
+ }
+ }
+
+ // Claim every role before doing I/O, and finish every load we own
before waiting for another
+ // request. Waiting first could deadlock requests owning opposite halves
of a role batch.
+ List<RolePolicyLoadKey> pendingLoads =
+ ownedLoads.stream()
+ .filter(key -> needsRolePolicyLoad(key.roleVersion))
+ .collect(Collectors.toList());
+ // Recheck after claiming: a request can observe a stale marker, be
delayed, and become the
+ // owner after the previous owner has already completed and removed its
flight.
+ Set<Long> unreadableRoleIds = loadRolePolicies(metalake, pendingLoads,
requestContext);
Review Comment:
[Important] Could we complete these futures per role rather than only after
the entire owned batch finishes? If a later role throws during resolution, the
catch below completes every owned future exceptionally, including roles whose
policies were already installed successfully.
I reproduced this with deterministic latches: request X owns loads for A and
B; request Y holds only A and joins A's future. A's policies and loaded-version
marker are installed successfully, then B's metadata resolution throws. Y
receives B's exception despite needing only the healthy, fully loaded A. In a
baseline comparison without this patch, Y completes successfully. This
introduces failure coupling across requests with different held-role sets.
Please preserve each role's individual outcome so a failure in B cannot turn
A's successful load into a failed future, while retaining the rule that callers
finish their owned work before waiting on other owners. A regression with an
owner of [A, B] and a waiter for [A] would cover this gap; the existing
disjoint-role and same-role failure tests do not exercise it.
--
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]
