HDDS-13532. Refactor BucketEndpoint.get() by rich7420 · Pull Request #10045 · apache/ozone

rich7420 · 2026-04-05T03:53:21Z

What changes were proposed in this pull request?

Reopen: #9110
This pull request refactors the massively monolithic BucketEndpoint.get() method (ListObjects API implementation) to vastly improve codebase maintainability, readability, and unit-testability.

The original get() method was nearly 200 lines long. It tightly coupled logic across multiple domains—from parameter string deserialization, context variable instantiation, iterative key-prefix filtering logic, to S3 metrics/audit log emissions. This structure made it inherently risky to extend and nearly impossible to write isolated unit tests for individual components of the logic.

Key Refactoring Improvements in this PR:

Context Encapsulation (BucketListingContext): Introduced a dedicated package-private class to securely encapsulate listing state (such as bucketName, prefix, encodingType, maxKeys, decodedToken, ozoneKeyIterator, etc.). This successfully prevented "parameter-creep" and minimized variable clutter across the execution flow.
Method Extraction & Separation of Concerns: We systematically decomposed the unmaintainable block into multiple single-responsibility helper methods:
- validateAndPrepareParameters(): Sanitizes configuration limits, handles token decodings, verifies encoding types, and enforces S3 Bucket owner conditions.
- initializeListObjectResponse(): Generates the bare-bones boilerplate response structures.
- processKeyListing(): Owns the complex computational burden (the while (ozoneKeyIterator.hasNext()) mechanism), resolving token depth matches, delimited deep structures, and CommonPrefixes allocations safely.
- buildFinalResponse(): Isolates the responsibility of executing PerformanceStringBuilder increments and consistent auditReadSuccess publishing.
Seamless Upstream Synchronization: During refactoring, this branch was comprehensively synced and merged with master's recent additions (S3RequestContext, PerformanceStringBuilder, and the BucketOperationHandler Chain-of-Responsibility pattern). The refactor cleanly embraces these integrations while achieving optimal flow footprint.

What is the link to the Apache JIRA?

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

How was this patch tested?

https://github.com/rich7420/ozone/actions/runs/23993539910

sreejasahithi

Thanks @rich7420

sreejasahithi · 2026-04-06T13:28:02Z

+    // 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


nit : this comment is repeated.

sreejasahithi · 2026-04-06T13:50:49Z

+      // Actual bucket processing starts here
+      // Validate and prepare parameters
+      BucketListingContext listingContext = validateAndPrepareParameters(
+          bucketName, delimiter, encodingType, marker, maxKeys, prefix, 
+          continueToken, startAfter);

-      // If continuation token and start after both are provided, then we
-      // ignore start After
-      String prevKey = continueToken != null ? decodedToken.getLastKey()
-          : startAfter;
+      // Initialize response object
+      ListObjectResponse response = initializeListObjectResponse(
+          bucketName, delimiter, encodingType, marker, maxKeys, prefix, 
+          continueToken, startAfter);


Here maxKeys is initialized from the query parameters (e.g., a user requests 5000), validateAndPrepareParameters is called. Inside this method, maxKeys is capped via validateMaxKeys(maxKeys) (e.g., capped to 1000). This capped value is stored inside the BucketListingContext, the maxKeys variable back in the get() method is not updated. It remains 5000. initializeListObjectResponse is called using the original maxKeys (5000).The response object now explicitly states 5000.

I think if we pass listingContext.getMaxKeys() not the local maxKeys to initializeListObjectResponse this issue would not be seen.

Could you confirm whether that’s intentional or something we should align?

rich7420 · 2026-04-07T04:38:53Z

@sreejasahithi Thanks for catching these!
I've removed the redundant comment and updated initializeListObjectResponse to properly use the capped listingContext.getMaxKeys(). The PR is updated.

sreejasahithi · 2026-04-22T05:10:45Z

@rich7420 could you please resolve the conflicts in this branch.

adoroszlai · 2026-04-22T06:14:51Z

+    @SuppressWarnings("parameternumber")
+    BucketListingContext(String bucketName, String delimiter, String encodingType,
+                               String marker, int maxKeys, String prefix, String continueToken,
+                               String startAfter, String prevKey, boolean shallow,
+                               OzoneBucket bucket, Iterator<? extends OzoneKey> ozoneKeyIterator,
+                               ContinueToken decodedToken) {


Instead of passing too many parameters, BucketListingContext should initialize itself directly from queryParams.

adoroszlai · 2026-04-22T06:17:43Z

+  @SuppressWarnings({"parameternumber", "checkstyle:ParameterNumber"})
+  BucketListingContext validateAndPrepareParameters(
+      String bucketName, String delimiter, String encodingType, String marker,
+      int maxKeys, String prefix, String continueToken, String startAfter)
+      throws OS3Exception, IOException {


Please move validation into BucketListingContext, which should perform it on the values read from queryParams.

adoroszlai · 2026-04-22T06:20:54Z

+   * Context class to hold bucket listing parameters and state.
+   */
+  static class BucketListingContext {


I'd prefer BucketListing as name, to not increase the number of "contexts" in S3 Gateway.

I think it can also be a top-level class.

adoroszlai · 2026-04-22T06:21:33Z

+  @SuppressWarnings({"parameternumber", "checkstyle:ParameterNumber"})
+  ListObjectResponse initializeListObjectResponse(
+      String bucketName, String delimiter, String encodingType, String marker,
+      int maxKeys, String prefix, String continueToken, String startAfter) {


Please move the following methods into BucketListing(Context), and use member variables:

initializeListObjectResponse

processKeyListing

buildFinalResponse

Also, please update the new unit test to exercise BucketListing only, without involving BucketEndpoint. (testGetDelegatesToAclHandler can be dropped.)

adoroszlai · 2026-04-23T14:44:21Z

-public class BucketEndpoint extends BucketOperationHandler {
+public class BucketEndpoint extends EndpointBase {


Please verify conflict resolution. The current state reverts ab71c6a.

@adoroszlai thanks for the reminder!

rich7420 · 2026-05-11T05:43:41Z

@adoroszlai @sreejasahithi just try to refactor the changes
could you plz take a look to confirm the changes is right or not? thanks

adoroszlai

Thanks @rich7420 for improving the patch.

adoroszlai · 2026-05-11T10:08:08Z

-  @Override
-  Response handlePutRequest(S3RequestContext context, String bucketName, InputStream body) {
-    throw newError(S3ErrorTable.NOT_IMPLEMENTED, "PUT bucket");
-  }
-


Please don't remove this.

adoroszlai · 2026-05-11T10:08:14Z

-  @Override
-  Response handleDeleteRequest(S3RequestContext context, String bucketName) {
-    throw newError(S3ErrorTable.NOT_IMPLEMENTED, "DELETE bucket");
-  }
-


Please don't remove this.

ok sorry about that

adoroszlai · 2026-05-11T10:08:29Z

        OZONE_S3G_LIST_MAX_KEYS_LIMIT,
        OZONE_S3G_LIST_MAX_KEYS_LIMIT_DEFAULT);

-    // initialize handlers


nit: unnecessary change

ok sorry about that

adoroszlai · 2026-05-11T10:08:38Z

-      getMetrics().updateGetBucketFailureStats(context.getStartNanos());
+      getMetrics().updateGetBucketFailureStats(startNanos);


nit: unnecessary change

adoroszlai · 2026-05-11T10:12:08Z

+      return BucketListing.fromQueryParams(this, bucketName)
+          .buildResponse(startNanos, perf);


Pass S3RequestContext context to BucketListing:

use it to access volume, which may be cached already

no need to pass individual parameters startNanos, perf

adoroszlai · 2026-05-11T10:13:31Z

+    shallow = endpoint.isListKeysShallowEnabled()
+        && OZONE_URI_DELIMITER.equals(delimiter);
+
+    bucket = endpoint.getVolume().getBucket(bucketName);


context.getVolume() can be used here (after passing S3RequestContext)

adoroszlai · 2026-05-11T10:14:43Z

-      getMetrics().updateGetBucketFailureStats(context.getStartNanos());
-      if (ex.getResult() == ResultCodes.FILE_NOT_FOUND) {
-        // File not found, continue and send normal response with 0 keyCount
-        LOG.debug("Key Not found prefix: {}", prefix);
-      } else {
-        throw ex;
+      getMetrics().updateGetBucketFailureStats(startNanos);
+      if (isAccessDenied(ex)) {
+        throw newError(S3ErrorTable.ACCESS_DENIED, bucketName, ex);
      }
+      throw ex;


Seems like unintended change.

adoroszlai · 2026-05-11T18:21:25Z

    } catch (OMException ex) {
      getMetrics().updateGetBucketFailureStats(context.getStartNanos());
-      if (ex.getResult() == ResultCodes.FILE_NOT_FOUND) {
-        // File not found, continue and send normal response with 0 keyCount
-        LOG.debug("Key Not found prefix: {}", prefix);
-      } else {
-        throw ex;
-      }
+      throw ex;


Since FILE_NOT_FOUND is handled within BucketListing, we don't need separate catch (OMException) anymore.

yandrey321 · 2026-05-11T19:21:32Z

+    bucket = context.getVolume().getBucket(bucketName);
+    S3Owner.verifyBucketOwnerCondition(endpoint.getHeaders(), bucketName, bucket.getOwner());
+    try {
+      ozoneKeyIterator = bucket.listKeys(prefix, prevKey, shallow);


do we really need to call listKeys() in validateAndPrepare(), it seems like an expensive call where results are discared.

I think ozoneKeyIterator is stored and consumed in processKeyListing(). also, listKeys() returns a lazy iterator no keys are fetched until the iterator is traversed.

yandrey321 · 2026-05-11T19:22:36Z

+    return Math.min(value, endpoint.getMaxKeysLimit());
+  }
+
+  private ListObjectResponse initializeListObjectResponse() {


should this code be moved into builder?

the function fromQueryParams() initializes all state,buildResponse() is the terminal step. I think adding a separate builder layer would increase complexity , isn't it?

sreejasahithi · 2026-05-17T05:37:14Z

Can you please add a class level javadoc here.

sreejasahithi · 2026-05-17T05:37:46Z

+
+    if (count < maxKeys) {
+      response.setTruncated(false);
+    } else if (ozoneKeyIterator.hasNext() && lastKey != null) {


Better to put a null check on ozoneKeyIterator here.

sreejasahithi · 2026-05-17T05:40:42Z

+    this.delimiter = endpoint.queryParams().get(QueryParams.DELIMITER);
+    this.encodingType = endpoint.queryParams().get(QueryParams.ENCODING_TYPE);
+    this.marker = endpoint.queryParams().get(QueryParams.MARKER);
+    this.maxKeys = endpoint.queryParams().getInt(QueryParams.MAX_KEYS, 1000);


Can we use OZONE_S3G_LIST_MAX_KEYS_LIMIT_DEFAULT here instead?

github-actions · 2026-06-08T00:29:02Z

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.

Extract bucket listing logic from BucketEndpoint.handleGetRequest() into a top-level BucketListing class that initializes itself from queryParams, performs its own validation, and encapsulates initializeListObjectResponse, processKeyListing, and buildFinalResponse as member methods.

…guard ozoneKeyIterator

github-actions · 2026-06-30T00:26:47Z

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.

github-actions · 2026-07-08T00:20:32Z

Thank you for your contribution. This PR is being closed due to inactivity. Please contact a maintainer if you would like to reopen it.

sreejasahithi reviewed Apr 6, 2026

View reviewed changes

rich7420 force-pushed the HDDS-13532 branch from 943675b to ba657c7 Compare April 7, 2026 04:37

sarvekshayr self-requested a review April 22, 2026 05:10

adoroszlai reviewed Apr 22, 2026

View reviewed changes

adoroszlai marked this pull request as draft April 23, 2026 14:36

adoroszlai reviewed Apr 23, 2026

View reviewed changes

rich7420 force-pushed the HDDS-13532 branch from 4f2b3e3 to 413098a Compare May 11, 2026 05:40

adoroszlai reviewed May 11, 2026

View reviewed changes

rich7420 force-pushed the HDDS-13532 branch from 413098a to 631032e Compare May 11, 2026 14:33

adoroszlai reviewed May 11, 2026

View reviewed changes

yandrey321 reviewed May 11, 2026

View reviewed changes

rich7420 force-pushed the HDDS-13532 branch 3 times, most recently from 5e21548 to b2ad3f4 Compare May 12, 2026 02:16

adoroszlai requested review from peterxcli and sreejasahithi and removed request for sreejasahithi May 16, 2026 17:10

sreejasahithi reviewed May 17, 2026

View reviewed changes

github-actions Bot added the stale label Jun 8, 2026

rich7420 force-pushed the HDDS-13532 branch from b2ad3f4 to dc82c68 Compare June 8, 2026 08:22

HDDS-13532. Address review: add BucketListing class javadoc and null-…

3d39380

…guard ozoneKeyIterator

github-actions Bot removed the stale label Jun 9, 2026

github-actions Bot added the stale label Jun 30, 2026

github-actions Bot closed this Jul 8, 2026

		public class BucketEndpoint extends BucketOperationHandler {
		public class BucketEndpoint extends EndpointBase {

		getMetrics().updateGetBucketFailureStats(context.getStartNanos());
		getMetrics().updateGetBucketFailureStats(startNanos);

		return BucketListing.fromQueryParams(this, bucketName)
		.buildResponse(startNanos, perf);

Uh oh!

Conversation

rich7420 commented Apr 5, 2026

What changes were proposed in this pull request?

What is the link to the Apache JIRA?

How was this patch tested?

Uh oh!

sreejasahithi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sreejasahithi Apr 6, 2026 • 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 Apr 7, 2026

Uh oh!

sreejasahithi commented Apr 22, 2026

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!

rich7420 commented May 11, 2026

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!

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!

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!

github-actions Bot commented Jun 8, 2026

Uh oh!

github-actions Bot commented Jun 30, 2026

Uh oh!

github-actions Bot commented Jul 8, 2026

sreejasahithi Apr 6, 2026 •

edited

Loading