yuqi1129 commented on code in PR #13699:
URL: https://github.com/apache/gravitino/pull/13699#discussion_r4236234158
##########
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:
Acknowledged. I am keeping the per-catalog monitor as an accepted tradeoff
in this PR. It preserves the tested bound of one failed initialization across
concurrent lookups of distinct objects. With a cached wrapper the critical
section covers capability retrieval and name normalization; cold initialization
already waits on Caffeine's catalog loader. The Javadoc explicitly documents
serialization. A lock-free healthy path would change the concurrency protocol,
so I will keep this patch focused rather than introduce that optimization
without profiling evidence.
##########
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:
Fixed in 90558d5199. Removed only @VisibleForTesting from
catalogScopedTypes(), keeping the method and its Javadoc unchanged. The other
testing-only helper retains its annotation. This is a one-line deletion in one
source file, with no behavior change. Spotless passes, and all 454
server-common non-Docker tests passed on rerun (the initial run hit the
previously observed TLS exception-shape failure).
--
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]
