Skip to content

HDDS-14361. Implement bucket CRUD as BucketOperationHandler#9679

Merged
adoroszlai merged 5 commits intoapache:masterfrom
Russole:HDDS-14361
Jan 28, 2026
Merged

HDDS-14361. Implement bucket CRUD as BucketOperationHandler#9679
adoroszlai merged 5 commits intoapache:masterfrom
Russole:HDDS-14361

Conversation

@Russole
Copy link
Contributor

@Russole Russole commented Jan 27, 2026

What changes were proposed in this pull request?

  • Refactor bucket PUT handling using BucketOperationHandler.
  • Move bucket create and delete logic into BucketCrudHandler.

What is the link to the Apache JIRA

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

How was this patch tested?

All CI checks passed.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Russole for the patch, mostly LGTM.

Comment on lines +52 to +54
return queryParams().get(QueryParams.ACL) == null
&& queryParams().get(QueryParams.UPLOADS) == null
&& queryParams().get(QueryParams.DELETE) == null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handlers are tried in order, so BucketCrudHandler does not need to check conditions, just handle all requests that reach it, like BucketEndpoint did.

We can keep this method if you prefer, but then please rename to shouldHandle and reuse in handleDeleteRequest.

@Russole
Copy link
Contributor Author

Russole commented Jan 27, 2026

Thanks @adoroszlai for the suggestion. I’ve renamed the method to shouldHandle() and applied it to both handlers.

@Russole Russole requested a review from adoroszlai January 27, 2026 15:32
@adoroszlai adoroszlai added the s3 S3 Gateway label Jan 27, 2026
Copy link
Contributor

@echonesis echonesis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Russole for the patch.
The refactoring looks good and follows the existing handler pattern well. However, could you please add unit tests for BucketCrudHandler?

You can refer to TestBucketAclHandler as an example. The tests should cover:

  • Normal bucket creation and deletion
  • Scenarios where shouldHandle() returns false (with query parameters)
  • Error handling cases (BUCKET_NOT_EMPTY, BUCKET_NOT_FOUND, INVALID_BUCKET_NAME)

@adoroszlai
Copy link
Contributor

could you please add unit tests for BucketCrudHandler?

Let's leave that for a follow-up. Existing tests for BucketEndpoint cover that now.

Copy link
Contributor

@echonesis echonesis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +1

@adoroszlai adoroszlai merged commit f8c9152 into apache:master Jan 28, 2026
44 checks passed
@adoroszlai
Copy link
Contributor

Thanks @Russole for the patch, @echonesis for the review.

@Russole
Copy link
Contributor Author

Russole commented Jan 28, 2026

Thanks @adoroszlai and @echonesis for the review.

@Russole Russole deleted the HDDS-14361 branch January 28, 2026 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s3 S3 Gateway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants