adoroszlai commented on code in PR #9710:
URL: https://github.com/apache/ozone/pull/9710#discussion_r2777637755
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectTaggingHandler.java:
##########
@@ -65,6 +68,41 @@ Response handlePutRequest(ObjectRequestContext context,
String keyName, InputStr
}
}
+ @Override
+ Response handleDeleteRequest(ObjectRequestContext context, String keyName)
+ throws IOException, OS3Exception {
+ if (context.ignore(getAction())) {
+ return null;
+ }
+ try {
+ context.getBucket().deleteObjectTagging(keyName);
+ } catch (OMException ex) {
+
getMetrics().updateDeleteObjectTaggingFailureStats(context.getStartNanos());
Review Comment:
`updateDeleteObjectTaggingFailureStats` should be called for any exception,
not just `OMException`.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectTaggingHandler.java:
##########
@@ -65,6 +68,41 @@ Response handlePutRequest(ObjectRequestContext context,
String keyName, InputStr
}
}
+ @Override
+ Response handleDeleteRequest(ObjectRequestContext context, String keyName)
+ throws IOException, OS3Exception {
+ if (context.ignore(getAction())) {
+ return null;
+ }
+ try {
+ context.getBucket().deleteObjectTagging(keyName);
+ } catch (OMException ex) {
+
getMetrics().updateDeleteObjectTaggingFailureStats(context.getStartNanos());
+ if (ex.getResult() == ResultCodes.KEY_NOT_FOUND) {
+ throw S3ErrorTable.newError(S3ErrorTable.NO_SUCH_KEY, keyName);
+ }
+ throw ex;
+ }
+
getMetrics().updateDeleteObjectTaggingSuccessStats(context.getStartNanos());
+ return Response.noContent().build();
Review Comment:
As an improvement (to make this method's structure similar to other handler
methods), please move these into `try` after `deleteObjectTagging`.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -371,33 +371,53 @@ public Response get(
@PathParam(BUCKET) String bucketName,
@PathParam(PATH) String keyPath
) throws IOException, OS3Exception {
+ ObjectRequestContext context =
+ new ObjectRequestContext(S3GAction.GET_KEY, bucketName);
Review Comment:
nit: this can be one line
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -371,33 +371,53 @@ public Response get(
@PathParam(BUCKET) String bucketName,
@PathParam(PATH) String keyPath
) throws IOException, OS3Exception {
+ ObjectRequestContext context =
+ new ObjectRequestContext(S3GAction.GET_KEY, bucketName);
+ try {
+ return handler.handleGetRequest(context, keyPath);
+ } catch (OMException ex) {
+ if (ex.getResult() == ResultCodes.KEY_NOT_FOUND) {
+ throw newError(S3ErrorTable.NO_SUCH_KEY, keyPath, ex);
+ } else if (isAccessDenied(ex)) {
+ throw newError(S3ErrorTable.ACCESS_DENIED, keyPath, ex);
+ } else if (ex.getResult() == ResultCodes.BUCKET_NOT_FOUND) {
+ throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);
+ } else {
+ throw ex;
+ }
+ }
+ }
+
+ @Override
+ @SuppressWarnings("checkstyle:MethodLength")
Review Comment:
`SuppressWarnings` does not need to be added, and can be removed from
`get()` as well.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -685,46 +676,23 @@ public Response delete(
@PathParam(BUCKET) String bucketName,
@PathParam(PATH) String keyPath
) throws IOException, OS3Exception {
- final String taggingMarker = queryParams().get(QueryParams.TAGGING);
- final String uploadId = queryParams().get(QueryParams.UPLOAD_ID);
-
- long startNanos = Time.monotonicNowNanos();
- S3GAction s3GAction = S3GAction.DELETE_KEY;
-
+ ObjectRequestContext context =
+ new ObjectRequestContext(S3GAction.DELETE_KEY, bucketName);
Review Comment:
nit: this can be one line
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1273,35 +1262,7 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
- private Response getObjectTagging(OzoneBucket bucket, String keyName) throws
IOException {
- long startNanos = Time.monotonicNowNanos();
-
- Map<String, String> tagMap = bucket.getObjectTagging(keyName);
-
- getMetrics().updateGetObjectTaggingSuccessStats(startNanos);
- return Response.ok(S3Tagging.fromMap(tagMap),
MediaType.APPLICATION_XML_TYPE).build();
- }
-
- private Response deleteObjectTagging(OzoneVolume volume, String bucketName,
String keyName)
- throws IOException, OS3Exception {
- long startNanos = Time.monotonicNowNanos();
-
- try {
- volume.getBucket(bucketName).deleteObjectTagging(keyName);
- } catch (OMException ex) {
- // Unlike normal key deletion that ignores the key not found exception
- // DeleteObjectTagging should throw the exception if the key does not
exist
Review Comment:
nit: please keep this comment (in
`ObjectTaggingHandler.handleDeleteRequest`).
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -733,22 +701,40 @@ public Response delete(
} else {
throw ex;
}
+ }
+ }
+
+ @Override
+ Response handleDeleteRequest(ObjectRequestContext context, String keyPath)
+ throws IOException, OS3Exception {
+
+ final String bucketName = context.getBucketName();
+ final long startNanos = context.startNanos;
+ final String uploadId = queryParams().get(QueryParams.UPLOAD_ID);
+
+ try {
+ OzoneVolume volume = context.getVolume();
+
+ if (uploadId != null && !uploadId.isEmpty()) {
+ context.setAction(S3GAction.ABORT_MULTIPART_UPLOAD);
+ Response r = abortMultipartUpload(volume, bucketName, keyPath,
uploadId);
+
+ return r;
Review Comment:
nit:
```suggestion
return abortMultipartUpload(volume, bucketName, keyPath, uploadId);
```
--
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]
