HDDS-13937. Make WithObjectID immutable by echonesis · Pull Request #9327 · apache/ozone

echonesis · 2025-11-19T16:02:15Z

What changes were proposed in this pull request?

This PR makes WithObjectID immutable.

Essential modifications:

Add @Immutable to WithObjectID
Remove setObjectID and setUpdateID. Instead, add Builder.withObjectID and Builder.withUpdateID to support the same logic

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13937

How was this patch tested?

GitHub Actions CI: https://github.com/echonesis/ozone/actions/runs/19562613361
Local Check

$ hadoop-ozone/dev-support/checks/findbugs.sh -Dspotbugs.version=4.8.3.0
$ grep -c 'WithObjectID' target/findbugs/summary.txt
0

adoroszlai

Thanks @echonesis for the patch.

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OmUtils.java

adoroszlai · 2025-11-19T16:33:09Z

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithObjectID.java

+    /** Hook method, customized in subclasses. */
+    public String getObjectInfo() {


getObjectInfo is overridden in value subclasses, but not in builders. What's more, value classes override toString(), but builders do not.

Probably worth a separate sub-task to keep patches from growing huge.

Thanks @adoroszlai
I agree with you.
Could I create a subtask under HDDS-10519 since HDDS-13937 has already been a sub-task?

Yes, subtask of HDDS-10519.

I've created one sub-task, HDDS-13964, to handle this.

...-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAclRequest.java

...one-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMRecoverLeaseRequest.java

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java

...-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java

...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java

...java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java

...n/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/om/PrefixParser.java

adoroszlai

Thanks @echonesis for updating the patch.

adoroszlai · 2025-11-20T17:52:50Z

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java

+      Map<String, String> metadataMap = omKeyInfo.toBuilder().getMetadata();
+      metadataMap.putAll(KeyValueUtil.getFromProtobuf(
+                commitKeyArgs.getMetadataList()));
+      omKeyInfo = omKeyInfo.toBuilder()


Avoid duplicate toBuilder():

Suggested change

Map<String, String> metadataMap = omKeyInfo.toBuilder().getMetadata();

metadataMap.putAll(KeyValueUtil.getFromProtobuf(

commitKeyArgs.getMetadataList()));

omKeyInfo = omKeyInfo.toBuilder()

OmKeyInfo.Builder builder = omKeyInfo.toBuilder();

Map<String, String> metadataMap = builder.getMetadata();

metadataMap.putAll(KeyValueUtil.getFromProtobuf(

commitKeyArgs.getMetadataList()));

omKeyInfo = builder

adoroszlai · 2025-11-20T18:03:57Z

...manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetOwnerRequest.java

-      newOmBucketInfo.setUpdateID(transactionLogIndex);
+      newOmBucketInfo = newOmBucketInfo.toBuilder()
+          .withUpdateID(transactionLogIndex)
+          .build();


Please move .withUpdateID into existing builder chain:

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetOwnerRequest.java

Lines 150 to 153 in bcd3b76

OmBucketInfo newOmBucketInfo = omBucketInfo.toBuilder()

.setOwner(newOwner)

.setModificationTime(setBucketPropertyRequest.getModificationTime())

.build();

adoroszlai · 2025-11-20T18:34:36Z

...manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OmKeysDeleteRequestWithFSO.java

+      omKeyInfo = omKeyInfo.toBuilder()
+          .withUpdateID(trxnLogIndex)
+          .build();


This updated omKeyInfo is not stored anywhere, so the updateID will be lost. We need to replace the item in the list.

adoroszlai · 2025-11-20T18:36:28Z

...manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OmKeysDeleteRequestWithFSO.java

+      omKeyInfo = omKeyInfo.toBuilder()
+          .withUpdateID(trxnLogIndex)
+          .build();


Same here, need to update dirList.

adoroszlai · 2025-11-20T20:12:06Z

...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java

+      dbKeyInfo = dbKeyInfo.toBuilder()
+          .withUpdateID(transactionLogIndex)
+          .build();
      dbKeyInfo.setReplicationConfig(replicationConfig);

      // Construct a new metadata map from KeyArgs by rebuilding via toBuilder.


Please combine these two builders.

adoroszlai · 2025-11-20T20:16:22Z

...-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java

+      Map<String, String> metaDataMap = omKeyInfo.toBuilder().getMetadata();
+      metaDataMap.putAll(KeyValueUtil.getFromProtobuf(
+              commitKeyArgs.getMetadataList()));
      omKeyInfo.setDataSize(commitKeyArgs.getDataSize());

      List<OmKeyLocationInfo> uncommitted =
          omKeyInfo.updateLocationInfoList(locationInfoList, false);

      // Set the UpdateID to current transactionLogIndex
-      omKeyInfo.setUpdateID(trxnLogIndex);
+      omKeyInfo = omKeyInfo.toBuilder()


Please avoid duplicate toBuilder().

adoroszlai · 2025-11-21T09:20:20Z

Thanks @echonesis for updating the patch, 6506a7d looks good.

a10e72a was unnecessary. This PR is already large, it does not need to move all mutations into the builder chains.

echonesis · 2025-11-21T09:34:54Z

Thanks @echonesis for updating the patch, 6506a7d looks good.

a10e72a was unnecessary. This PR is already large, it does not need to move all mutations into the builder chains.

OK, got it. I will revert to 6506a7d

adoroszlai · 2025-11-21T12:52:00Z

Thanks @echonesis for the patch.

echonesis · 2025-11-21T13:30:11Z

Thanks @adoroszlai for the review and merge.

HDDS-13937. Make WithObjectID immutable

96f15c7

adoroszlai reviewed Nov 19, 2025

View reviewed changes

echonesis added 2 commits November 20, 2025 23:53

fix: code review update

53f0d3e

chore: remove dummys

bcd3b76

adoroszlai reviewed Nov 20, 2025

View reviewed changes

echonesis added 2 commits November 21, 2025 13:56

fix: remove builder duplicates

9a1703c

fix: checkstyle fix

6506a7d

echonesis requested a review from adoroszlai November 21, 2025 08:10

echonesis force-pushed the HDDS-13937 branch from a10e72a to 6506a7d Compare November 21, 2025 09:35

adoroszlai approved these changes Nov 21, 2025

View reviewed changes

adoroszlai mentioned this pull request Nov 21, 2025

HDDS-13940. Make OmVolumeArgs owner/timestamps/quotas immutable #9305

Merged

adoroszlai merged commit 56e7988 into apache:master Nov 21, 2025
87 checks passed

echonesis deleted the HDDS-13937 branch November 21, 2025 13:30

		/** Hook method, customized in subclasses. */
		public String getObjectInfo() {

	OmBucketInfo newOmBucketInfo = omBucketInfo.toBuilder()
	.setOwner(newOwner)
	.setModificationTime(setBucketPropertyRequest.getModificationTime())
	.build();

Conversation

echonesis commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echonesis Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Nov 21, 2025

Uh oh!

echonesis commented Nov 21, 2025

Uh oh!

Uh oh!

adoroszlai commented Nov 21, 2025

Uh oh!

echonesis commented Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

echonesis commented Nov 19, 2025 •

edited

Loading

echonesis Nov 20, 2025 •

edited

Loading