HDDS-13532. Refactor BucketEndpoint.get() method #9110

rich7420 · 2025-10-07T07:04:56Z

What changes were proposed in this pull request?

This PR refactors the BucketEndpoint.get() method to improve code maintainability and readability.
The original method was nearly 200 lines long and contained multiple unrelated logic blocks, making it difficult to understand, test, and maintain.

Changes in this PRr

Extracted 5 methods from the get() method
add BucketListingContext class to encapsulate parameters
added test coverage (unit, integration, performance)

What is the link to the Apache JIRA

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

How was this patch tested?

Unit tests: TestBucketEndpointRefactoredSimple.java
Integration tests: TestBucketEndpointIntegration.java
Performance tests: TestBucketEndpointPerformance.java

peterxcli

Thanks @rich7420 for the patch. Please enable the build-branch workflow in your fork.
#9102 (review)

BTW, curious why we need these three new tests?

priyeshkaratha · 2025-10-07T09:08:50Z

Thanks, @rich7420, for the patch. Please move the integration tests to the integration module instead of keeping them with the unit tests. Also, it would be helpful if you could add some context explaining the purpose of these test cases.

rich7420 · 2025-10-07T11:59:02Z

@peterxcli , @priyeshkaratha Thanks for the review!
The three new tests were added mainly to verify the refactored logic in BucketEndpoint.get(). I think that they help ensure that behavior remains consistent before and after the refactor.
That said, I can move the integration test to the integration module as suggested and update the comments to clarify the purpose of each test.
If you prefer, I can also remove the tests after verifying it locally.

rich7420 · 2025-10-07T13:28:52Z

I’ve enabled the build-branch workflow on my fork. https://github.com/rich7420/ozone/actions/runs/18313906976/job/52148987227
Regarding the three tests: I trimmed the change set to keep the PR focused on the refactor. The newly added integration/performance tests have been removed to keep CI green and to avoid duplicating coverage that already exists in the integration suites. If the team prefers, I can reintroduce them in a follow‑up PR.

jojochuang · 2025-10-08T19:10:11Z

...ay/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketEndpointRefactoredSimple.java

+/**
+ * Simple unit tests for the refactored BucketEndpoint methods.
+ */
+public class TestBucketEndpointRefactoredSimple {


suggest to rename it as TestBucketEndpoint

jojochuang · 2025-10-08T19:18:25Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java

-        AUDIT.logReadSuccess(
-            buildAuditMessageForSuccess(s3GAction, getAuditParameters()));
-        return Response.ok(result, MediaType.APPLICATION_XML_TYPE).build();
+        return handleAclRequest(bucketName, startNanos);


suggest to rename as handleGetBucketAcl because it implements GetBucketAcl https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetBucketAcl.html

jojochuang · 2025-10-08T19:32:59Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java

+  /**
+   * Process key listing logic.
+   */
+  void processKeyListing(BucketListingContext context, ListObjectResponse response) {


if this method doesn't touch anything inside BucketEndpoint, and that it accesses BucketListingContext, then perhaps it's better placed inside BucketListingContext.

hevinhsu

Thanks @rich7420 for updating the patch. Overall LGTM. Left some comments.

hevinhsu · 2025-10-09T03:49:43Z

...ay/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketEndpointRefactoredSimple.java

+    bucketEndpoint = new BucketEndpoint();
+    OzoneClientStub client = new OzoneClientStub();
+
+    bucketEndpoint.setClient(client);
+    bucketEndpoint.setRequestIdentifier(new RequestIdentifier());
+    bucketEndpoint.setOzoneConfiguration(new OzoneConfiguration());
+    bucketEndpoint.init();


Maybe we can use a builder to simplify the endpoint creation:

Suggested change

bucketEndpoint = new BucketEndpoint();

OzoneClientStub client = new OzoneClientStub();

bucketEndpoint.setClient(client);

bucketEndpoint.setRequestIdentifier(new RequestIdentifier());

bucketEndpoint.setOzoneConfiguration(new OzoneConfiguration());

bucketEndpoint.init();

OzoneClientStub client = new OzoneClientStub();

bbucketEndpoint = EndpointBuilder.newBucketEndpointBuilder()

.setClient(client)

.build();

hevinhsu · 2025-10-09T03:52:30Z

...ay/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketEndpointRefactoredSimple.java

+    bucketEndpoint.init();
+
+    // Create a test bucket
+    client.getObjectStore().createS3Bucket("test-bucket");


Looks like all test cases use the same bucketName. Maybe we can extract it into a shared variable.

TaiJuWu · 2025-10-09T08:03:10Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java

-    // If you specify the encoding-type request parameter,should return
-    // encoded key name values in the following response elements:
-    //   Delimiter, Prefix, Key, and StartAfter.
-    //
-    // For detail refer:
-    // https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html
-    // #AmazonS3-ListObjectsV2-response-EncodingType
-    //


should this comment be keep？

rich7420 · 2025-10-13T13:57:21Z

@jojochuang @hevinhsu @TaiJuWu Thanks a lot for the suggestions! I’ve implemented the updates that switched tests to use EndpointBuilder for endpoint creation, moved the key-listing logic into BucketListingContext with BucketEndpoint.processKeyListing(...) delegating to it, renamed the method to handleGetBucketAcl(...), added throws IOException to the test helper getObjectEndpoint(...), and fixed the trailing newline and related style checks. CI is all green now. If there’s anything else we can refine, I’m happy to follow up.

hevinhsu

Thanks @rich7420 for continuously improving this patch. Overall it LGTM, I've just left a few comments below.

hevinhsu · 2025-11-11T02:53:49Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java

      @QueryParam("upload-id-marker") String uploadIdMarker,
      @DefaultValue("1000") @QueryParam("max-uploads") int maxUploads) throws OS3Exception, IOException {
    long startNanos = Time.monotonicNowNanos();
    S3GAction s3GAction = S3GAction.GET_BUCKET;


Nit: Maybe we can move this line to where the actual bucket processing starts, for better readability — similar to how you declared s3GAction at the beginning of the actual process block in handleGetBucketAcl and listMultipartUploads.

hevinhsu · 2025-11-11T03:08:10Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java

+    // Return empty response if no keys found
+    ListObjectResponse emptyResponse = new ListObjectResponse();
+    emptyResponse.setName(bucketName);
+    emptyResponse.setKeyCount(0);
+    return Response.ok(emptyResponse).build();


It seems you chose another approach to handle the FILE_NOT_FOUND case when no key is found.
Previously, the exception was caught and the get bucket process continued, ensuring the audit log was written afterward.

In your version, this logic is handled in a separate block that returns an empty response, but it seems the audit log is not written in this path.
Maybe we should also log the audit entry here to align with the previous behavior.

github-actions · 2025-12-03T00:04:48Z

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

jojochuang

I'm not sure about this.
In a typical refactoring effort, the behavior of the code change should stay exactly the same. However, it doesn't look like so especially the exception handling.

jojochuang · 2025-12-05T21:00:40Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java

+    // Handle ACL requests
+    if (aclMarker != null) {
+      return handleGetBucketAcl(bucketName, startNanos);
+    }

-      maxKeys = validateMaxKeys(maxKeys);
+    // Handle multipart uploads requests
+    if (uploads != null) {
+      return listMultipartUploads(bucketName, prefix, keyMarker, uploadIdMarker, maxUploads);
+    }

-      if (prefix == null) {
-        prefix = "";
-      }
+    // Actual bucket processing starts here
+    S3GAction s3GAction = S3GAction.GET_BUCKET;


these lines should go into the try block below.

rich7420 · 2025-12-06T03:00:29Z

@jojochuang Thanks for the review! I'll fix it as soon as possible

github-actions · 2025-12-28T00:06:08Z

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

adoroszlai · 2026-01-06T21:34:58Z

Thanks @rich7420 for working on this. In HDDS-14123 we started to move logic into separate handler classes. E.g. BucketAclHandler takes care of bucket ACL creation/retrieval (put is implemented, get to be done in HDDS-14360). The end goal is to have BucketEndpoint just delegate to a list of handlers.

github-actions · 2026-01-28T00:06:11Z

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

peterxcli requested review from ivandika3, jojochuang and peterxcli October 7, 2025 07:58

peterxcli added the s3 S3 Gateway label Oct 7, 2025

peterxcli reviewed Oct 7, 2025

View reviewed changes

jojochuang reviewed Oct 8, 2025

View reviewed changes

hevinhsu reviewed Oct 9, 2025

View reviewed changes

TaiJuWu reviewed Oct 9, 2025

View reviewed changes

rich7420 force-pushed the HDDS-13532 branch from 67ece5a to e6e0311 Compare October 13, 2025 15:52

adoroszlai requested review from TaiJuWu, hevinhsu and jojochuang November 7, 2025 15:45

hevinhsu reviewed Nov 11, 2025

View reviewed changes

hevinhsu approved these changes Nov 11, 2025

View reviewed changes

github-actions bot added the stale label Dec 3, 2025

jojochuang reviewed Dec 5, 2025

View reviewed changes

github-actions bot removed the stale label Dec 6, 2025

github-actions bot added the stale label Dec 28, 2025

rich7420 force-pushed the HDDS-13532 branch from 4a1b041 to 61a6cc1 Compare December 28, 2025 09:25

github-actions bot removed the stale label Dec 29, 2025

github-actions bot added the stale label Jan 28, 2026

rich7420 added 9 commits January 28, 2026 14:25

Refactor BucketEndpoint.get()

98d2580

move test

27b332e

fix error

6898667

remove IT test for passing build-branch

f51ba60

rename methids and test class

cae5e0b

keep comment

5659135

add audit log entry

c72c279

move if position

6499067

Refactor BucketEndpoint.get() into helper methods

032ba32

rich7420 force-pushed the HDDS-13532 branch from 61a6cc1 to 032ba32 Compare January 28, 2026 06:55

HDDS-13532. Refactor BucketEndpoint.get() method #9110

Are you sure you want to change the base?

HDDS-13532. Refactor BucketEndpoint.get() method #9110

Conversation

rich7420 commented Oct 7, 2025

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

peterxcli left a comment

Choose a reason for hiding this comment

Uh oh!

priyeshkaratha commented Oct 7, 2025

Uh oh!

rich7420 commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rich7420 commented Oct 7, 2025

Uh oh!

jojochuang Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

jojochuang Oct 8, 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!

jojochuang Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

hevinhsu left a comment

Choose a reason for hiding this comment

Uh oh!

hevinhsu Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

hevinhsu Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

TaiJuWu Oct 9, 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!

rich7420 commented Oct 13, 2025

Uh oh!

hevinhsu left a comment

Choose a reason for hiding this comment

Uh oh!

hevinhsu Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

hevinhsu Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 3, 2025

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

jojochuang Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

rich7420 commented Dec 6, 2025

Uh oh!

github-actions bot commented Dec 28, 2025

Uh oh!

adoroszlai commented Jan 6, 2026

Uh oh!

github-actions bot commented Jan 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rich7420 commented Oct 7, 2025 •

edited

Loading

jojochuang Oct 8, 2025 •

edited

Loading

TaiJuWu Oct 9, 2025 •

edited

Loading