Copilot commented on code in PR #10792:
URL: https://github.com/apache/ozone/pull/10792#discussion_r3605905640
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractRootedOzoneFileSystemTest.java:
##########
@@ -1847,6 +1847,56 @@ void testGetFileStatus() throws Exception {
fs.delete(volume, false);
}
+ @Test
+ void testGetFileStatusUsesSingleOmRpcAfterCacheWarm() throws Exception {
+ Path path = new Path(bucketPath, "single-rpc-stat-test");
+ fs.mkdirs(path);
+ // Warm the bucket layout cache; ignore metrics from the first stat.
+ fs.getFileStatus(path);
+
+ long getFileStatusBefore = getOMMetrics().getNumGetFileStatus();
+ long bucketInfoBefore = getOMMetrics().getNumBucketInfos();
+
+ FileStatus status = fs.getFileStatus(path);
+ assertTrue(status.isDirectory());
+
+ assertEquals(getFileStatusBefore + 1,
getOMMetrics().getNumGetFileStatus());
+ assertEquals(bucketInfoBefore, getOMMetrics().getNumBucketInfos());
+ }
+
+ @Test
+ void testGetFileStatusOnBucketRoot() throws Exception {
+ FileStatus status = fs.getFileStatus(bucketPath);
+ assertTrue(status.isDirectory());
+ }
+
+ @Test
+ void testGetFileStatusOnObjectStoreBucketRejectsInvalidLayout()
+ throws Exception {
+ OzoneBucket obsBucket =
+ TestDataUtil.createVolumeAndBucket(client, BucketLayout.OBJECT_STORE);
+ Path obsBucketPath = new Path(
+ new Path(OZONE_URI_DELIMITER + obsBucket.getVolumeName()),
+ obsBucket.getName());
+ try {
+ IllegalArgumentException exception = assertThrows(
+ IllegalArgumentException.class, () ->
fs.getFileStatus(obsBucketPath));
+ assertThat(exception.getMessage())
+ .contains(BucketLayout.OBJECT_STORE.name());
+ } finally {
+ objectStore.deleteVolume(obsBucket.getVolumeName());
+ }
Review Comment:
The cleanup in `testGetFileStatusOnObjectStoreBucketRejectsInvalidLayout`
deletes the volume but never deletes the bucket created by
`TestDataUtil.createVolumeAndBucket(...)`. `ObjectStore#deleteVolume` delegates
to OM and typically fails when the volume is non-empty, making the test cleanup
flaky/leaky. Delete the bucket first, then the volume (consistent with other
tests in this class).
##########
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 a non-atomic `containsKey` + RPC pattern.
Under concurrent access, multiple threads can race and each issue
`getBucketDetails`, reintroducing redundant OM calls and partially defeating
the intended optimization. Using `computeIfAbsent` (with checked-exception
wrapping) makes the cache population atomic per bucket.
--
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]
