Copilot commented on code in PR #10792:
URL: https://github.com/apache/ozone/pull/10792#discussion_r3606133579
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -274,6 +280,47 @@ OzoneBucket getBucket(OFSPath ofsPath, boolean
createIfNotExist)
createIfNotExist);
}
+ private static String bucketLayoutCacheKey(String volumeStr, String
bucketStr) {
+ return volumeStr + OZONE_URI_DELIMITER + bucketStr;
+ }
+
+ private BucketLayout validateBucketLayoutForBucket(OzoneBucket bucket)
+ throws IOException {
+ BucketLayout resolvedBucketLayout =
+ OzoneClientUtils.resolveLinkBucketLayout(bucket, objectStore,
+ new HashSet<>());
+ OzoneFSUtils.validateBucketLayout(bucket.getName(), resolvedBucketLayout);
+ return resolvedBucketLayout;
+ }
+
+ private void cacheValidatedBucketLayout(String volumeStr, String bucketStr,
+ BucketLayout layout) {
+ validatedBucketLayouts.putIfAbsent(
+ bucketLayoutCacheKey(volumeStr, bucketStr), layout);
+ }
+
+ private void validateAndCacheBucketLayout(String volumeStr, String bucketStr,
+ OzoneBucket bucket) throws IOException {
+ BucketLayout layout = validateBucketLayoutForBucket(bucket);
+ cacheValidatedBucketLayout(volumeStr, bucketStr, layout);
+ }
+
+ private void resolveAndValidateBucketLayout(String volumeStr, String
bucketStr)
+ throws IOException {
+ OzoneBucket bucket = proxy.getBucketDetails(volumeStr, bucketStr);
+ validateAndCacheBucketLayout(volumeStr, bucketStr, bucket);
+ }
+
+ private void ensureBucketLayoutValid(OFSPath ofsPath) throws IOException {
+ String volumeStr = ofsPath.getVolumeName();
+ String bucketStr = ofsPath.getBucketName();
+ if (validatedBucketLayouts.containsKey(
+ bucketLayoutCacheKey(volumeStr, bucketStr))) {
+ return;
+ }
+ resolveAndValidateBucketLayout(volumeStr, bucketStr);
+ }
Review Comment:
ensureBucketLayoutValid() uses containsKey() followed by a separate
getBucketDetails()+putIfAbsent(). Under concurrent access on a cold bucket,
multiple threads can observe the cache miss and all issue redundant
getBucketDetails (GetBucketInfo) RPCs, which undermines the goal of avoiding
extra OM calls. Using computeIfAbsent() makes the cache population atomic and
ensures at most one RPC per (volume,bucket) when warming the cache.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -132,6 +133,11 @@ public class BasicRootedOzoneClientAdapterImpl
private BucketLayout defaultOFSBucketLayout;
private final OzoneConfiguration config;
private final OzoneClientConfig clientConfig;
+ /**
+ * Cache of successfully validated bucket layouts for read paths.
+ */
+ private final ConcurrentHashMap<String, BucketLayout> validatedBucketLayouts
=
+ new ConcurrentHashMap<>();
Review Comment:
validatedBucketLayouts is an unbounded cache that grows monotonically with
every distinct (volume,bucket) accessed by this client process. In long-lived
OzoneFS clients that touch many buckets (e.g., gateway services or multi-tenant
jobs), this can become an unbounded memory growth vector. Consider using a
bounded/expiring cache (e.g., Guava Cache with maximumSize/expireAfterAccess)
or otherwise limiting growth/invalidation.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
