jerryshao commented on code in PR #13699:
URL: https://github.com/apache/gravitino/pull/13699#discussion_r4236208254
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizationLookups.java:
##########
@@ -131,11 +102,99 @@ public Optional<OwnerInfo> resolveOwnerId(
id -> ownerRelCache.get(id, k -> loadOwner(k, metadataType)));
}
+ // Preserve normalization failures as distinct from missing objects for
DENY-policy consumers.
+ MetadataIdResolution resolveMetadataIdResult(
+ MetadataObject metadataObject, String metalake,
AuthorizationRequestContext requestContext) {
+ MetadataObject cacheObject;
+ try {
+ // Use the same capability rules as ID resolution so hooks and peer
change-log replay
+ // evict every equivalent spelling from both cache tiers.
+ String rawKey =
JcasbinAuthorizationCacheKeys.metadataIdCacheKey(metalake, metadataObject);
+ Function<String, MetadataObject> normalizer =
+ ignored -> {
+ try {
+ return
MetadataIdConverter.normalizeMetadataObject(metadataObject, metalake);
+ } catch (NotFoundException e) {
+ throw e;
+ } catch (RuntimeException e) {
+ LOG.warn(
+ "Cannot normalize metadata object {}:{} in metalake {};
authorization lookup is unresolved",
+ metadataObject.type(),
+ metadataObject.fullName(),
+ metalake,
+ e);
+ throw e;
+ }
+ };
+ if
(MetadataIdConverter.catalogScopedTypes().contains(metadataObject.type())) {
Review Comment:
[Nit] This line makes a `@VisibleForTesting` method a production dependency.
`catalogScopedTypes()` is annotated `@VisibleForTesting`
(`server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataIdConverter.java:139-142`),
so the annotation now misstates the contract: the method is on the request
path, not just in the mapping-derived test parameterization. Dropping the
annotation and keeping the javadoc would say what it has become.
Verified by: grep for `catalogScopedTypes` across the repo - this line is
the only production caller, plus the suites in
`TestJcasbinMetadataIdCacheNormalization`.
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java:
##########
@@ -208,6 +218,56 @@ public Optional<GroupUpdatedAt> computeGroupInfoIfAbsent(
k -> Objects.requireNonNull(loader.apply(k), "Group info loader must
not return null"));
}
+ /**
+ * Normalizes each raw metadata name at most once per request.
+ *
+ * <p>The key must include the metalake, object type and raw name.
Successful results are a
+ * request-local snapshot; failures are not cached, so a later lookup may
retry. A fresh request
+ * must resolve current catalog rules again. This map must not be shared
across mutations.
+ *
+ * @param key the raw metadata object's cache key
+ * @param loader the normalization function, which must return a non-null
object or throw
+ * @return the canonical metadata object
+ */
+ public MetadataObject computeNormalizedMetadataObjectIfAbsent(
+ String key, Function<String, MetadataObject> loader) {
+ return normalizedMetadataObjects.computeIfAbsent(
+ key,
+ k -> Objects.requireNonNull(loader.apply(k), "Normalization loader
must not return null"));
+ }
+
+ /**
+ * Normalizes a raw name while remembering capability failures for its
catalog in this request.
+ *
+ * <p>Catalog lookups are serialized so concurrent objects cannot retry a
failed initialization.
+ * Missing catalogs are not memoized; a fresh request can retry any
capability failure.
+ *
+ * @param key the raw metadata object's cache key
+ * @param catalogIdentifier the containing catalog, including its metalake
+ * @param loader the normalization function, which must return a non-null
object or throw
+ * @return the canonical metadata object
+ * @throws RuntimeException if normalization failed for this catalog
+ */
+ public MetadataObject computeNormalizedMetadataObjectIfAbsent(
+ String key, NameIdentifier catalogIdentifier, Function<String,
MetadataObject> loader) {
+ CatalogNormalizationState state =
+ catalogNormalizationStates.computeIfAbsent(
+ catalogIdentifier, ignored -> new CatalogNormalizationState());
+ synchronized (state) {
Review Comment:
[Nit] The per-catalog monitor is held across every normalization, so the
healthy path is serialized too.
`MetadataAuthzHelper.doFilter` submits one task per entity on a shared
executor, all tasks sharing this one request context
(`server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java:540-566`),
and the entities of a list response normally live in one catalog. So after
this change every task's normalization of a *distinct* object queues on the
same monitor, not just the failing ones.
The cost looks small: the critical section is
`CapabilityHelpers.getCapability` -> `CatalogManager.doWithCatalog` ->
`catalogCache.get` plus `classLoader.withClassLoader(c -> c.capability())`
(`core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:815-826`,
`:354-356`), which is in-memory once the wrapper is cached, and the cold case
would block on Caffeine's per-key load anyway. So this is a note, not a request.
If you want the healthy path lock-free while keeping the bound that
`testConcurrentCatalogFailuresAreRememberedAcrossObjects` pins: make `failure`
volatile, read it before taking the monitor, and enter the monitor only when a
first attempt for that catalog is still outstanding.
Verified by: reading this method and `doFilter` in this checkout, following
`getCapability` into `doWithCatalog`/`loadCatalogAndWrapInternal`, and grepping
`server-common/src/main/java/org/apache/gravitino/server/authorization/` for
`parallelStream|CompletableFuture|ExecutorService` - `doFilter` is the only
intra-request concurrency.
--
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]
