[GitHub] [ozone] adoroszlai commented on a diff in pull request #3263: HDDS-6528. Adding Unit-Test cases for S3-Gateway Bucket-Endpoint Metrics.

[GitBox <gi...@apache.org>]

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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org