HDDS-15925. Avoid redundant getBucketInfo RPC on OFS getFileStatus#10792
HDDS-15925. Avoid redundant getBucketInfo RPC on OFS getFileStatus#10792jojochuang wants to merge 7 commits into
Conversation
Cache validated bucket layouts per volume/bucket and call GetFileStatus directly after cache warm-up, preserving layout checks and headOp behavior. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: Icddbc99fee0cf0e78e52919283f93d29a1986a1c
There was a problem hiding this comment.
Pull request overview
This PR optimizes ofs:// getFileStatus() by caching per-bucket layout validation in BasicRootedOzoneClientAdapterImpl, so repeated getFileStatus() calls can avoid an extra getBucketInfo/getBucketDetails round-trip and rely on a single OM GetFileStatus RPC after warm-up.
Changes:
- Added a
(volume, bucket)-scoped in-memory cache for validated bucket layouts and wired it into the bucket-resolution path. - Updated non-snapshot
getFileStatusto validate bucket layout once (cached) and then callproxy.getOzoneFileStatus(...)directly. - Added integration tests asserting RPC-count behavior after cache warm-up and covering bucket-root stats, OBS-bucket layout rejection, and missing-file behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java | Introduces cached bucket-layout validation and routes getFileStatus through a single OM RPC after warm-up. |
| hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractRootedOzoneFileSystemTest.java | Adds integration coverage for the single-RPC behavior and additional getFileStatus edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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); | ||
| } |
| } finally { | ||
| objectStore.deleteVolume(obsBucket.getVolumeName()); | ||
| } |
Delete the bucket before removing the volume to avoid VOLUME_NOT_EMPTY. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: Iddfefe48d716e77cb58501514c919a44194cdae7
| 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); | ||
| } |
| /** | ||
| * Cache of successfully validated bucket layouts for read paths. | ||
| */ | ||
| private final ConcurrentHashMap<String, BucketLayout> validatedBucketLayouts = | ||
| new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
it might be the case for HBase, so I'd agree with Copilot that we need TTL/eviction policy and size limit for the cache.
Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: Ifc177a61594da562cc3cbc13b6f622fada55971d
Mock ClientProtocol and warm the layout cache after getFileStatus stopped calling OzoneBucket.getFileStatus for non-snapshot paths. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I34e369396f682c096fa23b0f5df7b8b165feab6f
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:756
- validatedBucketLayouts is never invalidated. If a bucket is deleted and later re-created (potentially with a different layout), a cached entry can cause future getFileStatus() calls to skip layout validation and bypass OBJECT_STORE rejection. At minimum, clear the cache entry when OM reports BUCKET_NOT_FOUND so the next call revalidates against the current bucket definition.
} catch (OMException e) {
if (e.getResult() == OMException.ResultCodes.FILE_NOT_FOUND) {
throw new FileNotFoundException(key + ": No such file or directory!");
} else if (e.getResult() == OMException.ResultCodes.BUCKET_NOT_FOUND) {
throw new FileNotFoundException(key + ": Bucket doesn't exist!");
| Field cacheField = | ||
| BasicRootedOzoneClientAdapterImpl.class.getDeclaredField( | ||
| "validatedBucketLayouts"); | ||
| cacheField.setAccessible(true); | ||
| ConcurrentHashMap<String, BucketLayout> cache = | ||
| new ConcurrentHashMap<>(); | ||
| cache.put(volumeName + "/" + bucketName, BucketLayout.FILE_SYSTEM_OPTIMIZED); | ||
| cacheField.set(adapter, cache); |
|
@yandrey321 this PR aims to remove the extra RPC call in most cases. |
|
this is a good fix for reducing number of RPC calls for metadata, we can extend this fix or file a separate JIRA to route getFileChecksum, listStatus, getVolumeInfo and getBucket through the same cache. |
Call proxy.getOzoneFileStatus directly on non-snapshot paths and drop the client-side layout cache. Document accepted OBJECT_STORE stat behavior change. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I5c9df439cb636c7a0a2f0660f6bedce41b2b4419
| * | ||
| * <p>Non-snapshot paths call OM GetFileStatus directly (HDDS-15925) without a | ||
| * prior InfoBucket RPC. OBJECT_STORE buckets are not rejected on this path; | ||
| * mutating OFS operations still validate layout via {@link #getBucket(OFSPath, boolean)}. | ||
| */ | ||
| private FileStatusAdapter getFileStatusForKeyOrSnapshot( | ||
| OFSPath ofsPath, URI uri, Path qualifiedPath, String userName, | ||
| boolean headOp) throws IOException { | ||
| String key = ofsPath.getKeyName(); | ||
| try { | ||
| OzoneBucket bucket = getBucket(ofsPath, false); | ||
| if (ofsPath.isSnapshotPath()) { | ||
| OzoneBucket bucket = getBucket(ofsPath, false); | ||
| OzoneVolume volume = objectStore.getVolume(ofsPath.getVolumeName()); | ||
| return getFileStatusAdapterWithSnapshotIndicator( | ||
| volume, bucket, uri); | ||
| } else { | ||
| OzoneFileStatus status = bucket.getFileStatus(key, headOp); | ||
| OzoneFileStatus status = proxy.getOzoneFileStatus(ofsPath.getVolumeName(), | ||
| ofsPath.getBucketName(), key, headOp); | ||
| return toFileStatusAdapter(status, userName, uri, qualifiedPath, |
…atus. After skipping InfoBucket, OM BUCKET_NOT_FOUND is mapped in the adapter as today for GetFileStatus paths. Use try/finally so volume cleanup runs on assertion. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I2408951221c741ae24b823c69847ac905d1bd9f8
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:702
- The Javadoc says non-snapshot getFileStatus no longer rejects OBJECT_STORE buckets, but the PR description explicitly says OBJECT_STORE rejection and link-bucket layout validation are unchanged. Since the non-snapshot path now bypasses getBucket(..), any layout validation in getBucket() will not run here. Either update the PR description to reflect the intended behavior change, or restore layout validation for this path (ideally without reintroducing an extra InfoBucket RPC).
*
* <p>Non-snapshot paths call OM GetFileStatus directly (HDDS-15925) without a
* prior InfoBucket RPC. OBJECT_STORE buckets are not rejected on this path;
* mutating OFS operations still validate layout via {@link #getBucket(OFSPath, boolean)}.
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:716
- Non-snapshot getFileStatus now calls proxy.getOzoneFileStatus(...) directly without first resolving link-bucket layout or validating bucket layout (OBJECT_STORE rejection) as getBucket(...) does. This changes behavior: getFileStatus can succeed (or fail differently) on OBJECT_STORE/link buckets where OFS previously rejected file-system semantics early via OzoneFSUtils.validateBucketLayout(). Consider validating the bucket layout at least once per (volume,bucket) (eg. a small bounded cache) before calling GetFileStatus, so the optimization doesn’t silently drop this semantics check.
} else {
OzoneFileStatus status = proxy.getOzoneFileStatus(ofsPath.getVolumeName(),
ofsPath.getBucketName(), key, headOp);
Rooted OFS getFileStatus on keys no longer rejects OBJECT_STORE layout; ls on an OBS key succeeds while mutating operations still fail as before. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: Icfacbc5e8d61b32a9c548a7ed95188dee3cc2925
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:718
- getFileStatusForKeyOrSnapshot() now calls proxy.getOzoneFileStatus() for bucket paths (where keyName is empty) as well as key paths, which bypasses the bucket-layout validation enforced by getBucket(). This can make OBJECT_STORE buckets appear as normal directories via getFileStatus/exists(), even though other OFS operations still reject OBJECT_STORE. Consider handling bucket paths separately: use getBucket(ofsPath,false) to validate layout and return a bucket FileStatusAdapter, while keeping the direct GetFileStatus RPC optimization only for key paths.
} else {
OzoneFileStatus status = proxy.getOzoneFileStatus(ofsPath.getVolumeName(),
ofsPath.getBucketName(), key, headOp);
return toFileStatusAdapter(status, userName, uri, qualifiedPath,
ofsPath.getNonKeyPath());
| * <p>Non-snapshot paths call OM GetFileStatus directly (HDDS-15925) without a | ||
| * prior InfoBucket RPC. OBJECT_STORE buckets are not rejected on this path; | ||
| * mutating OFS operations still validate layout via {@link #getBucket(OFSPath, boolean)}. |
| Verify ls succeeds on OBS bucket key | ||
| ${url} = Format FS URL ${SCHEME} ${volume} ${bucket} testfile | ||
| ${result} = Execute and checkrc ozone fs -ls ${url} 255 | ||
| Should contain ${result} Bucket: ${bucket} has layout: OBJECT_STORE, which does not support file system semantics. Bucket Layout must be FILE_SYSTEM_OPTIMIZED or LEGACY. |
There was a problem hiding this comment.
this is a breaking change. Interestingly, OM does not reject getFileStatus() call.
Without breaking this behavior, it would require a client side cache to store bucket info. But that's more complex.
What changes were proposed in this pull request?
Rooted OFS
getFileStatus()on normal bucket/key paths (non-snapshot) previously calledgetBucket()→ InfoBucket, thenbucket.getFileStatus(), which is alreadyproxy.getOzoneFileStatus(). This PR skips the redundant InfoBucket RPC and callsGetFileStatusonly viaproxy.getOzoneFileStatus(..., headOp).Snapshot indicator paths are unchanged: they still use
getBucket()and synthetic status (noGetFileStatus).Behavior change (accepted): Client-side
validateBucketLayout()no longer runs on the stat path. OM acceptsGetFileStatusfor OBJECT_STORE buckets, so rootedgetFileStatusmay succeed or returnFILE_NOT_FOUNDinstead of throwingIllegalArgumentException. Mutating OFS operations (mkdir,create,delete, etc.) still callgetBucket()and reject OBJECT_STORE as before.This is a client-side optimization for HDDS-15925. It is not the HDDS-11232 approach (no combined OM response / server-side merge of bucket info with file status). HDDS-15925 references HDDS-11232 for the same RPC-reduction theme.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15925
How was this patch tested?
TestBasicRootedOzoneClientAdapterHeadOp(directgetOzoneFileStatus/headOp)TestOFS#testGetFileStatusUsesSingleOmRpc(OM metrics: no InfoBucket increment,GetFileStatus+1 per stat)FileNotFoundException(client mapping)ozonefs-obs.robotupdated for OBS stat vs mutating opsGenerated-by: Cursor (Composer)