HDDS-14123. Refactor BucketEndpoint#Put method #9516
Conversation
There was a problem hiding this comment.
Thanks @echonesis for working on this. I would like to simplify the implementation, but first need to do some refactoring in S3G to make it easier. (#9519 done, #9522 PR ready, HDDS-14204 in progress, HDDS-14198, maybe more)
| * Context object that provides access to BucketEndpoint resources. | ||
| * This allows handlers to access endpoint functionality without | ||
| * tight coupling to the BucketEndpoint class. | ||
| * | ||
| * Since BucketEndpoint extends EndpointBase, handlers can access: | ||
| * - Bucket and Volume operations | ||
| * - Methods inherited from EndpointBase | ||
| */ | ||
| public class BucketEndpointContext { |
There was a problem hiding this comment.
I don't think we should introduce BucketEndpointContext. The context duplicates "random" functionality of BucketEndpoint. It would likely need to grow when extracting other PUT operations (tagging, etc.), GET operations. We will also want to extract handlers from ObjectEndpoint.
Handlers can be considered specialised "mini" endpoints. Thus they should simply extend EndpointBase to inherit all required functionality (configuration, headers, request context, audit logging, etc.).
| Map<String, String> queryParams = new HashMap<>(); | ||
| queryParams.put("acl", aclMarker); | ||
| // Future handlers: queryParams.put("lifecycle", lifecycleMarker); | ||
|
|
||
| // Check for subresource operations using handlers | ||
| String queryParam = HANDLER_FACTORY.findFirstSupportedQueryParam(queryParams); | ||
|
|
||
| if (queryParam != null) { | ||
| BucketOperationHandler handler = HANDLER_FACTORY.getHandler(queryParam); | ||
| // Delegate to specific handler | ||
| s3GAction = getActionForQueryParam(queryParam); | ||
| Response response = handler.handlePutRequest( | ||
| bucketName, body, headers, getContext(), startNanos); | ||
| AUDIT.logWriteSuccess( | ||
| buildAuditMessageForSuccess(s3GAction, getAuditParameters())); |
There was a problem hiding this comment.
I think this is too complex.
- Query params are already available in a map via
ContainerRequestContext. - Handlers should take care of audit logging and metrics. The logic to decide value of
s3GActionshould not be in centralized inBucketEndpoint.
It is better to let handlers inspect query params (and possibly other inputs, like headers) to decide if they should handle the request or not.
Something like (in AclHandler):
private boolean shouldHandle() {
return hasQueryParam(ACL);
}
public Response handlePutRequest(String bucketName, InputStream body) ... {
if (!shouldHandle()) {
return null;
}
... handle bucket ACL putThen we can simply try all handlers until one of them does the work:
for (BucketOperationHandler handler : handlers) {
Response response = handler.handlePutRequest(bucketName, body);
if (response != null) {
return response;
}
}
... continue handling in `BucketEndpoint`
|
Thanks @adoroszlai for the review. |
9 participants
What changes were proposed in this pull request?
Refactored the BucketEndpoint#put method by extracting ACL handling logic into dedicated handler classes, improving code modularity and maintainability through the Strategy pattern.
Changes
AclHandlerclass to handle bucket ACL PUT operationsBucketOperationHandlerinterface for pluggable operation handlersBucketEndpoint#put()to delegate ACL operations toAclHandlerTesting
TestAclHandlerTestBucketAcltestsWhat is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14123
How was this patch tested?
GitHub Actions CI: https://github.com/echonesis/ozone/actions/runs/20489170244