adoroszlai commented on code in PR #3263:
URL: https://github.com/apache/ozone/pull/3263#discussion_r846721582
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/metrics/S3GatewayMetrics.java:
##########
@@ -44,7 +44,9 @@
// BucketEndpoint
private @Metric MutableCounterLong getBucketSuccess;
+
Review Comment:
```suggestion
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/metrics/S3GatewayMetrics.java:
##########
@@ -44,7 +44,9 @@
// BucketEndpoint
private @Metric MutableCounterLong getBucketSuccess;
+
private @Metric MutableCounterLong getBucketFailure;
+
Review Comment:
```suggestion
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -507,6 +514,10 @@ public Response putAcl(String bucketName, HttpHeaders
httpHeaders,
LOG.error("Error in set ACL Request for bucket: {}", bucketName,
exception);
throw exception;
+ } catch (OS3Exception ex) {
+ getMetrics().incPutAclFailure();
+ LOG.debug("Failed to put acl of Bucket " + bucketName, ex);
Review Comment:
We don't need the debug message, because the exception is logged at debug
level in `newError`.
https://github.com/apache/ozone/blob/e81ab0a3f76b09072c23e0151c5e28673623ed6c/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java#L141-L145
```suggestion
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -145,6 +145,10 @@ public Response get(
} else {
throw ex;
}
+ } catch (OS3Exception ex) {
+ getMetrics().incGetBucketFailure();
+ LOG.debug("Bucket Does not Exist " + bucketName, ex);
+ throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);
Review Comment:
We should just rethrow `ex`, because:
* `OS3Exception` already has an error code and proper message. No need to
wrap in another exception.
* Cause may be different, not necessarily `NO_SUCH_BUCKET`.
On second thought we also don't need the debug message, because the
exception is logged at debug level in `newError`.
https://github.com/apache/ozone/blob/e81ab0a3f76b09072c23e0151c5e28673623ed6c/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java#L141-L145
```suggestion
throw ex;
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -240,7 +244,7 @@ public Response put(@PathParam("bucket") String bucketName,
if (exception.getResult() == ResultCodes.INVALID_BUCKET_NAME) {
throw newError(S3ErrorTable.INVALID_BUCKET_NAME, bucketName,
exception);
}
- LOG.error("Error in Create Bucket Request for bucket: {}", bucketName,
+ LOG.debug("Error in Create Bucket Request for bucket: {}", bucketName,
Review Comment:
Nit: unrelated change.
```suggestion
LOG.error("Error in Create Bucket Request for bucket: {}", bucketName,
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -407,6 +411,10 @@ public S3BucketAcl getAcl(String bucketName)
LOG.error("Failed to get acl of Bucket " + bucketName, ex);
throw newError(S3ErrorTable.INTERNAL_ERROR, bucketName, ex);
}
+ } catch (OS3Exception ex) {
+ getMetrics().incGetAclFailure();
+ LOG.debug("Failed to get acl of Bucket " + bucketName, ex);
+ throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);
Review Comment:
Same here:
```suggestion
throw ex;
```
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/metrics/TestS3GatewayMetrics.java:
##########
@@ -110,4 +123,146 @@ public void testHeadObject() throws Exception {
long curMetric = metrics.getHeadKeySuccess();
assertEquals(1L, curMetric - oriMetric);
}
-}
+
+ @Test
+ public void testGetBucket() throws Exception {
+ long oriMetric = metrics.getGetBucketSuccess();
+
+ clientStub = createClientWithKeys("file1");
+ bucketEndpoint.setClient(clientStub);
+ bucketEndpoint.get(bucketName, null,
+ null, null, 1000, null,
+ null, "random", null,
+ null, null).getEntity();
+
+ long curMetric = metrics.getGetBucketSuccess();
+ assertEquals(1L, curMetric - oriMetric);
+
+
+ oriMetric = metrics.getGetBucketFailure();
+
+ try {
+ // Searching for a bucket that does not exist
+ bucketEndpoint.get("newBucket", null,
+ null, null, 1000, null,
+ null, "random", null,
+ null, null);
+ fail();
+ } catch (OS3Exception e) {
+ }
+
+ curMetric = metrics.getGetBucketFailure();
+ assertEquals(1L, curMetric - oriMetric);
+ }
+
+ @Test
+ public void testCreateBucket() throws Exception {
+
+ long oriMetric = metrics.getCreateBucketSuccess();
+
+ bucketEndpoint.put(bucketName, null,
+ null, null);
+ long curMetric = metrics.getCreateBucketSuccess();
+
+ assertEquals(1L, curMetric - oriMetric);
+
+
+ // Creating an error by trying to create a bucket that already exists
+ oriMetric = metrics.getCreateBucketFailure();
+
+ bucketEndpoint.put(bucketName, null, null, null);
+
+ curMetric = metrics.getCreateBucketFailure();
+ assertEquals(1L, curMetric - oriMetric);
+ }
+
+ @Test
+ public void testDeleteBucket() throws Exception {
+ long oriMetric = metrics.getDeleteBucketSuccess();
+
+ bucketEndpoint.delete(bucketName);
+
+ long curMetric = metrics.getDeleteBucketSuccess();
+ assertEquals(1L, curMetric - oriMetric);
+
+
+ oriMetric = metrics.getDeleteBucketFailure();
+ try {
+ // Deleting a bucket that does not exist will result in delete failure
+ bucketEndpoint.delete(bucketName);
+ fail();
+ } catch (OS3Exception ex) {
+ assertEquals(S3ErrorTable.NO_SUCH_BUCKET.getCode(), ex.getCode());
+ assertEquals(S3ErrorTable.NO_SUCH_BUCKET.getErrorMessage(),
+ ex.getErrorMessage());
+ }
+
+ curMetric = metrics.getDeleteBucketFailure();
+ assertEquals(1L, curMetric - oriMetric);
+ }
+
+ @Test
+ public void testGetAcl() throws Exception {
+ long oriMetric = metrics.getGetAclSuccess();
+
+ Response response =
+ bucketEndpoint.get(bucketName, null, null,
+ null, 0, null, null,
+ null, null, "acl", null);
+ long curMetric = metrics.getGetAclSuccess();
+ assertEquals(HTTP_OK, response.getStatus());
+ assertEquals(1L, curMetric - oriMetric);
+
+
+ oriMetric = metrics.getGetAclFailure();
+ try {
+ // Failing the getACL endpoint by applying ACL on a non-Existent Bucket
+ bucketEndpoint.get("random_bucket", null,
+ null, null, 0, null,
+ null, null, null, "acl", null);
+ fail();
+ } catch (OS3Exception ex) {
+ assertEquals(S3ErrorTable.NO_SUCH_BUCKET.getCode(), ex.getCode());
+ assertEquals(S3ErrorTable.NO_SUCH_BUCKET.getErrorMessage(),
+ ex.getErrorMessage());
+ }
+ curMetric = metrics.getGetAclFailure();
+ assertEquals(1L, curMetric - oriMetric);
+ }
+
+ @Test
+ public void testPutAcl() throws Exception {
+ long oriMetric = metrics.getPutAclSuccess();
+
+ clientStub.getObjectStore().createS3Bucket("b1");
+ InputStream inputBody = TestBucketAcl.class.getClassLoader()
+ .getResourceAsStream("userAccessControlList.xml");
+
+ bucketEndpoint.put("b1", ACL_MARKER, headers, inputBody);
+
+ long curMetric = metrics.getPutAclSuccess();
+ assertEquals(1L, curMetric - oriMetric);
+
+ // Failing the putACL endpoint by applying ACL on a non-Existent Bucket
+ oriMetric = metrics.getPutAclFailure();
+
+ try {
+ bucketEndpoint.put("unknown_bucket", ACL_MARKER, headers, inputBody);
+ fail();
+ } catch (OS3Exception ex) {
+ }
+ curMetric = metrics.getPutAclFailure();
+ assertEquals(1L, curMetric - oriMetric);
+ }
+
+ private OzoneClient createClientWithKeys(String... keys) throws IOException {
+ OzoneClient client = new OzoneClientStub();
Review Comment:
`setup()` already creates a `clientStub`, why is this new `client` needed?
--
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]
