HDDS-16701. S3G: Avoid calling getS3Volume in DELETE_KEY by rich7420 · Pull Request #11740 · apache/ozone

rich7420 · 2026-10-24T15:28:28Z

What changes were proposed in this pull request?

Use the S3 client delegation introduced in #11757 to let OM resolve the volume during DeleteKey on supported versions, removing the separate GetS3VolumeContext call for S3 DeleteObject requests without expected-owner checks. Older OMs retain client-side resolution, and explicit volumes and conditional deletes are preserved.

What is the link to the Apache JIRA

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

How was this patch tested?

JDK 21: 605 unit cases, 6 tenant integration cases and 8 AWS SDK v1/v2 conditional-delete cases passed, with no failures, errors or skips. Mini-cluster data used an external disk to satisfy the default free-space threshold.
Checkstyle and PMD passed. The older-OM volume-resolution test failed with the adapter change removed and passed after restoring it.
Validation for cc09103f11: Fork CI and 10×10 TestS3ClientProtocol check passed.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

adoroszlai

Thanks @rich7420 for the patch.

Sorry for being a bit late to ask this question about the changes for HDDS-16602: do we really need to essentially duplicate the surface of ClientProtocol interface with <verb>S3<object> methods? Instead, can we create a new ClientProtocol implementation that delegates to another ClientProtocol instance (the RpcClient in prod), but simply ignores volumeName parameter and passes one of two values based on omVersion? Then S3 endpoints can keep using the existing interface, passing any constant value (e.g. OZONE_S3_VOLUME_NAME_DEFAULT for simplicity).

Sorry if this was already considered and not chosen earlier, did not follow all discussions.

CC @andyhuangdev @chungen0126 @Russole

adoroszlai · 2026-10-24T15:57:53Z

+    verifyBucketName(bucketName);
+    Objects.requireNonNull(keyName, "keyName == null");
+    OmKeyArgs keyArgs = new OmKeyArgs.Builder()
+        .setVolumeName(OzoneConfigKeys.OZONE_S3_VOLUME_NAME_DEFAULT)
+        .setBucketName(bucketName)
+        .setKeyName(keyName)
+        .setExpectedETag(expectedETag)
+        .setAssumeS3Context(true)
+        .build();
+    ozoneManagerClient.deleteKey(keyArgs);


Even if we continue expanding the interface with S3-specific methods, can we avoid duplicating the implementation? Can simply decide on the volume name to use based on OM version, and call existing deleteKey?

Thanks, I’ll try the delegating approach with deleteKey first, reusing the existing delete implementation. I’ll check how to carry assumeS3Context through that path while preserving the older-OM fallback.

@adoroszlai, does this ClientProtocol delegation match the approach you had in mind?

adoroszlai · 2026-10-24T18:15:33Z

+import org.apache.hadoop.ozone.client.protocol.ClientProtocol;
+
+/** Adapts client operations to the S3 volume context. */
+final class S3ClientProtocol implements InvocationHandler {


Thanks @rich7420 for experimenting with this. I would avoid reflection, simply let S3ClientProtocol implements ClientProtocol and generate the delegation.

Thanks, replaced reflection with explicit delegation, including all default methods

chungen0126 · 2026-10-25T04:42:25Z

Thanks @rich7420 for the patch.

Sorry for being a bit late to ask this question about the changes for HDDS-16602: do we really need to essentially duplicate the surface of ClientProtocol interface with <verb>S3<object> methods? Instead, can we create a new ClientProtocol implementation that delegates to another ClientProtocol instance (the RpcClient in prod), but simply ignores volumeName parameter and passes one of two values based on omVersion? Then S3 endpoints can keep using the existing interface, passing any constant value (e.g. OZONE_S3_VOLUME_NAME_DEFAULT for simplicity).

Sorry if this was already considered and not chosen earlier, did not follow all discussions.

CC @andyhuangdev @chungen0126 @Russole

Thanks @adoroszlai, this is a great idea!

However, introducing this change (S3ClientProtocol delegation) has a broader impact beyond this PR and affects multiple sub-tasks under HDDS-16602. I think it would be better to have @rich7420 file a separate JIRA to track and handle this design refactoring independently.

Additionally, passing a constant value (e.g. OZONE_S3_VOLUME_NAME_DEFAULT) makes more sense than null, as null volume names will likely be rejected by null checks.

adoroszlai · 2026-10-25T05:49:54Z

I think it would be better to have @rich7420 file a separate JIRA to track and handle this design refactoring independently.

Thanks @chungen0126, I totally agree. Sorry I had to pick a specific PR to mention this.

replaced reflection with explicit delegation

Thanks @rich7420, I think that's much better. Let's continue in a separate PR.

Copilot AI balanced review requested due to automatic review settings October 24, 2026 15:28

Copilot AI reviewed Oct 24, 2026

github-actions Bot added om client s3 S3 Gateway labels Oct 24, 2026

adoroszlai reviewed Oct 24, 2026

View reviewed changes

rich7420 marked this pull request as draft October 24, 2026 18:04

adoroszlai reviewed Oct 24, 2026

View reviewed changes

rich7420 marked this pull request as ready for review October 24, 2026 19:22

rich7420 marked this pull request as draft October 25, 2026 01:16

rich7420 marked this pull request as ready for review October 25, 2026 02:27

chungen0126 marked this pull request as draft October 25, 2026 05:30

rich7420 mentioned this pull request Oct 25, 2026

HDDS-17452. Introduce ClientProtocol delegation for S3 Gateway #11757

Merged

HDDS-16701. S3G: Avoid calling getS3Volume in DELETE_KEY

cc09103

rich7420 force-pushed the HDDS-16701 branch from 6e3267b to cc09103 Compare October 26, 2026 15:15

rich7420 marked this pull request as ready for review October 26, 2026 16:43

Navigation Menu

Repository navigation

HDDS-16701. S3G: Avoid calling getS3Volume in DELETE_KEY - #11740

HDDS-16701. S3G: Avoid calling getS3Volume in DELETE_KEY#11740
rich7420 wants to merge 1 commit into
apache:masterfrom
rich7420:HDDS-16701

rich7420 commented Oct 24, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

adoroszlai left a comment

Uh oh!

adoroszlai Oct 24, 2026

Uh oh!

rich7420 Oct 24, 2026

Uh oh!

rich7420 Oct 24, 2026

Uh oh!

adoroszlai Oct 24, 2026

Uh oh!

rich7420 Oct 25, 2026

Uh oh!

chungen0126 commented Oct 25, 2026

Uh oh!

adoroszlai commented Oct 25, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

rich7420 commented Oct 24, 2026 • 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

rich7420 Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

rich7420 Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

adoroszlai Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

rich7420 Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

chungen0126 commented Oct 25, 2026

Uh oh!

adoroszlai commented Oct 25, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rich7420 commented Oct 24, 2026 •

edited

Loading