adoroszlai commented on code in PR #3263:
URL: https://github.com/apache/ozone/pull/3263#discussion_r841604915
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -155,6 +155,10 @@ public Response get(
} else {
throw ex;
}
+ } catch (OS3Exception ex) {
+ getMetrics().incGetBucketFailure();
+ LOG.error("Bucket Does not Exist " + bucketName, ex);
Review Comment:
Non-existent bucket is not an error, we should not log it as such. Please
see HDDS-6206 and HDDS-6247 for effort to avoid such logging in S3 Gateway.
The same applies to `putAcl` and `getAcl`.
##########
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 testGetBucketEndpointMetric() throws Exception {
Review Comment:
Nit: why do all these new test cases have `EndpointMetric` in their names?
##########
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 testGetBucketEndpointMetric() throws Exception {
+ long oriMetric = metrics.getGetBucketSuccess();
+
+ clientStub = createClientWithKeys("file1");
+ bucketEndpoint.setClient(clientStub);
+ bucketEndpoint.get(bucketName, null,
+ null, null, 1000, null,
+ 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, null, "random", null,
+ null, null);
+ fail();
+ } catch (OS3Exception e) {
+ }
+
+ curMetric = metrics.getGetBucketFailure();
+ assertEquals(1L, curMetric - oriMetric);
+ }
Review Comment:
Can you please add separate test cases (methods) for success/failure
scenarios instead of bundling them in the same?
--
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]
