HDDS-10518. Create base Builder in WithMetadata and WithObjectID #6378
Conversation
|
@ivandika3 please take a look as well |
ivandika3
left a comment
ivandika3
left a comment
There was a problem hiding this comment.
Thank you for the patch. Mostly LGTM. Few minor comments.
| this.metadata = metadata; | ||
| addAllMetadata(metadata); |
There was a problem hiding this comment.
Nit: Should not affect current usage, but the semantic seems to be changed changed from "replacing" to "adding". Might need to clear the metadata before adding it or can change the method name to addAllMetadata (but OzoneBucket is client-facing class so might not be a good idea).
I'm OK if you decide not to change it at all.
There was a problem hiding this comment.
Good point. Updated to clear any existing metadata for these two classes.
|
|
||
| public Builder setMetadata(Map<String, String> metadata) { | ||
| this.metadata = metadata; | ||
| addAllMetadata(metadata); |
There was a problem hiding this comment.
Similar comment to OzoneBucket.Builder#setMetadata.
ivandika3
left a comment
ivandika3
left a comment
There was a problem hiding this comment.
Thank you for the update. LGTM +1.
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@adoroszlai , thanks for working on this! The change look good. I just have comments on adding @Override tag.
|
|
||
| public Builder setMetadata(Map<String, String> metadata) { | ||
| this.metadata = metadata; | ||
| super.setMetadata(metadata); |
|
|
||
| public Builder setMetadata(Map<String, String> metadata) { | ||
| this.metadata = metadata; | ||
| super.setMetadata(metadata); |
| public Builder addAllMetadata(Map<String, String> map) { | ||
| metadata.putAll(map); | ||
| super.addAllMetadata(map); |
| public Builder setObjectID(long obId) { | ||
| this.objectID = obId; | ||
| super.setObjectID(obId); |
| public Builder setUpdateID(long id) { | ||
| this.updateID = id; | ||
| super.setUpdateID(id); |
| * System. | ||
| * @param id - long | ||
| */ | ||
| public Builder setObjectID(long id) { |
| * increasing values which are updated each time there is an update. | ||
| * @param id - long | ||
| */ | ||
| public Builder setUpdateID(long id) { |
| return this; | ||
| } | ||
|
|
||
| public Builder addMetadata(String key, String value) { |
| return this; | ||
| } | ||
|
|
||
| public Builder addAllMetadata(Map<String, String> additionalMetaData) { |
| return this; | ||
| } | ||
|
|
||
| public OmPrefixInfo.Builder addMetadata(String key, String value) { |
|
Thanks @szetszwo for the review, added all missing |
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
|
Thanks @ivandika3, @szetszwo for the review. |
…bjectID (apache#6378) (cherry picked from commit 7feafe9) Conflicts: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketInfo.java hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmDirectoryInfo.java hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/helpers/OmPrefixInfo.java hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmPrefixInfo.java hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/common/CommonUtils.java Change-Id: Ie89b7ee52e0a8f33341700b03b28983895d72e55
3 participants
What changes were proposed in this pull request?
Builders in subclasses of
WithMetadataandWithObjectIDall have their own code for setting the properties from the base classes. The goal of this task is to reduce code duplication by creating builders in the base classes and using them in the subclasses.Existing subclass builder methods are kept for covariant return to be able to keep existing chained method calls (e.g. return
OmBucketInfoinstead ofWithMetadata). We can remove them after replacing with non-chained calls in a follow-up patch.https://issues.apache.org/jira/browse/HDDS-10518
How was this patch tested?
CI:
https://github.com/adoroszlai/ozone/actions/runs/8277449313