adoroszlai commented on code in PR #9516: URL: https://github.com/apache/ozone/pull/9516#discussion_r2630179704
########## hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpointContext.java: ########## @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.s3.endpoint; + +import java.io.IOException; +import org.apache.hadoop.ozone.audit.AuditAction; +import org.apache.hadoop.ozone.client.OzoneBucket; +import org.apache.hadoop.ozone.client.OzoneVolume; +import org.apache.hadoop.ozone.om.exceptions.OMException; +import org.apache.hadoop.ozone.s3.exception.OS3Exception; + +/** + * 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 { Review 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.). ########## hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java: ########## @@ -321,13 +330,25 @@ public Response put(@PathParam("bucket") String bucketName, S3GAction s3GAction = S3GAction.CREATE_BUCKET; try { - if (aclMarker != null) { - s3GAction = S3GAction.PUT_ACL; - Response response = putAcl(bucketName, body); + // Build map of query parameters + 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())); Review Comment: I think this is too complex. 1. Query params are already available in a map via `ContainerRequestContext`. 2. Handlers should take care of audit logging and metrics. The logic to decide value of `s3GAction` should not be in centralized in `BucketEndpoint`. 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`): ```java private boolean shouldHandle() { return hasQueryParam(ACL); } public Response handlePutRequest(String bucketName, InputStream body) ... { if (!shouldHandle()) { return null; } ... handle bucket ACL put ``` Then we can simply try all handlers until one of them does the work: ```java for (BucketOperationHandler handler : handlers) { Response response = handler.handlePutRequest(bucketName, body); if (response != null) { return response; } } ... continue handling in `BucketEndpoint` ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
