HDDS-15624. Resolve link bucket properties in listBucket.#10745
Conversation
(cherry picked from commit 228f523de68218907d4658aae0df59a1321a6f35)
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect operational properties returned for link buckets in listBuckets by applying the same server-side link resolution/enrichment that already exists for getBucketInfo. This prevents clients/UIs from misreporting layout/replication for linked buckets (eg showing cluster defaults instead of the source bucket’s properties).
Changes:
- Enrich
listBucketsresults by resolving link buckets and overlaying the source bucket’s operational properties. - Extract the link-bucket enrichment logic into a shared helper used by both
getBucketInfoandlistBuckets. - Add a unit test covering
listBucketslink resolution for both FSO and OBS bucket layouts (and their replication configs).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java | Enriches listBuckets responses for link buckets by resolving links and overlaying source bucket operational properties (also reused by getBucketInfo). |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestBucketManagerImpl.java | Adds a unit test ensuring listBuckets returns resolved layout/replication for both FSO and OBS link buckets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| List<OmBucketInfo> enrichedBuckets = new ArrayList<>(buckets.size()); | ||
| for (OmBucketInfo bucket : buckets) { | ||
| enrichedBuckets.add(enrichLinkBucketInfo(bucket)); | ||
| } | ||
| return enrichedBuckets; |
| } | ||
| metrics.incNumBucketLists(); | ||
| return bucketManager.listBuckets(volumeName, | ||
| List<OmBucketInfo> buckets = bucketManager.listBuckets(volumeName, |
There was a problem hiding this comment.
i don't like that the list buckets is a short lived object. it would be nice if we can reuse this list object.
There was a problem hiding this comment.
Updated the commit to reuse the list.
| OmBucketInfo realBucket = | ||
| bucketManager.getBucketInfo( | ||
| resolvedBucket.realVolume(), | ||
| resolvedBucket.realBucket()); | ||
| // Pass the real bucket metadata in the link bucket info. | ||
| return bucketInfo.toBuilder() | ||
| .setDefaultReplicationConfig( | ||
| realBucket.getDefaultReplicationConfig()) | ||
| .setIsVersionEnabled(realBucket.getIsVersionEnabled()) | ||
| .setStorageType(realBucket.getStorageType()) | ||
| .setQuotaInBytes(realBucket.getQuotaInBytes()) | ||
| .setQuotaInNamespace(realBucket.getQuotaInNamespace()) | ||
| .setUsedBytes(realBucket.getUsedBytes()) | ||
| .setSnapshotUsedBytes(realBucket.getSnapshotUsedBytes()) | ||
| .setSnapshotUsedNamespace(realBucket.getSnapshotUsedNamespace()) | ||
| .setUsedNamespace(realBucket.getUsedNamespace()) | ||
| .addAllMetadata(realBucket.getMetadata()) | ||
| .setBucketLayout(realBucket.getBucketLayout()) | ||
| .build(); |
There was a problem hiding this comment.
i think this is going to be hard to maintain in the future, if we add additional fields to the bucket.
What if we instantiate a bucketInfo object from realBucket, and update volumeName, bucketName instead.
There was a problem hiding this comment.
TBH i'm not sure that's a good suggestion or not.
There was a problem hiding this comment.
Moved the bucketInfo builder from the source closer to where the properties are added in OmBucketInfo.
…ce closer OmBucketInfo.
jojochuang
left a comment
jojochuang
left a comment
There was a problem hiding this comment.
ok i think this is good. will merge with the debug message suggestion from Copilot.
| * <p>When adding new operational bucket fields, update this method if they | ||
| * should be resolved from a link's source bucket. | ||
| */ | ||
| public OmBucketInfo withOperationalPropertiesFrom(OmBucketInfo source) { |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| .setSnapshotUsedBytes(source.getSnapshotUsedBytes()) | ||
| .setSnapshotUsedNamespace(source.getSnapshotUsedNamespace()) | ||
| .addAllMetadata(source.getMetadata()) | ||
| .setBucketLayout(source.getBucketLayout()) | ||
| .setTags(source.getTags()) | ||
| .build(); |
| } catch (IOException e) { | ||
| LOG.debug("Failed to enrich listBuckets entry for {}/{}; returning raw entry", volumeName, buckets.get(i).getBucketName(), e); | ||
| } |
…uckets Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: Ic58109e086074097ef3d5c9395174ffacd41caa0
|
@SaketaChalamchala I found two related issues with this (missing ACL check, didn't copy EncryptionKeyInfo). Filed HDDS-15961 for them. PTAL. PR: #10859 |
What changes were proposed in this pull request?
Link buckets created with
ozone sh bucket linkstore link metadata (sourceVolume, sourceBucket) plus placeholder operational fields (layout, replication, quotas, etc.).getBucketInfoalready resolved source properties server-side (since HDDS-9117), butlistBucketsreturned raw DB rows. UIs and clients that use operational fields off of the list buckets response show incorrect layout/replication for links (e.g. cluster default FSO instead of source OBS).This change applies the same server-side link resolution to
listBucketsby extracting shared enrichment logic used by bothgetBucketInfoandlistBuckets.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15624
How was this patch tested?
Unit Tests.