HDDS-6528. Adding Unit-Test cases for S3-Gateway Bucket-Endpoint Metrics. by ArafatKhan2198 · Pull Request #3263 · apache/ozone

ArafatKhan2198 · 2022-04-01T17:48:22Z

What changes were proposed in this pull request?

Added Unit Tests for Bucket Endpoint Metrics for S3-Gateway

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6528

How was this patch tested?

Wrote Unit tests for existing metrics

@jojochuang @aryangupta1998 @siddhantsangwan @adoroszlai

adoroszlai

Thanks @ArafatKhan2198 for working on this. There is compile error due to recent change on master (HDDS-6466). Can you please merge origin/master into your branch?

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java

...p-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/metrics/TestS3GatewayMetrics.java

adoroszlai · 2022-04-04T11:01:45Z

...p-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/metrics/TestS3GatewayMetrics.java

Can you please add separate test cases (methods) for success/failure scenarios instead of bundling them in the same?

Each method is associated with a single S3 Endpoint and there are 2 metrics associated with each endpoint, hence we are measuring the number of times a method succeeds and fails, and once an object is initialised we can reuse it for both the metrics, therefore I felt clubbing them together would make more sense !!

Each method is associated with a single S3 Endpoint and there are 2 metrics associated with each endpoint, hence we are measuring the number of times a method succeeds and fails, and once an object is initialised we can reuse it for both the metrics

Currently each test method is associated with a method on one of the endpoints. E.g. testCreateBucket tests bucketEndpoint.put(), while testDeleteBucket tests bucketEndpoint.delete(). If object reuse is so important, why not merge all these together in a huge testBucketEndpoint method?

The reasons for separating these are at least:

Small, self-contained test methods are easier to understand.

Independence: test cases may pass or fail regardless of the result of other test cases. If multiple cases are bundled in the same method, failure of one case may hide the result of the subsequent case.

We can apply the same reasons to testing different metrics on the same endpoint and method pairs.

Yes you are absolutely right on that part, I'll make the necessary changes
Thanks for the help !!

adoroszlai

Thanks @ArafatKhan2198 for updating the patch. On second look I've found a few more issues.

adoroszlai · 2022-04-10T05:43:11Z

...p-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/metrics/TestS3GatewayMetrics.java

+  }
+
+  private OzoneClient createClientWithKeys(String... keys) throws IOException {
+    OzoneClient client = new OzoneClientStub();


setup() already creates a clientStub, why is this new client needed?

I am sorry I forgot to remove it !!
will change it, thanks !!

adoroszlai · 2022-04-10T06:20:53Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java

+      LOG.debug("Bucket Does not Exist " + bucketName, ex);
+      throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);


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.

ozone/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java

Lines 141 to 145 in e81ab0a

if (e.getHttpCode() == HTTP_INTERNAL_ERROR) {

LOG.error("Internal Error: {}", err.toXml(), ex);

} else if (LOG.isDebugEnabled()) {

LOG.debug(err.toXml(), ex);

}

Suggested change

LOG.debug("Bucket Does not Exist " + bucketName, ex);

throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);

throw ex;

+1 we should just increment the counter and retrhwo

Sure will do !!

adoroszlai · 2022-04-10T06:21:19Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java

+      LOG.debug("Failed to get acl of Bucket " + bucketName, ex);
+      throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);


Same here:

Suggested change

LOG.debug("Failed to get acl of Bucket " + bucketName, ex);

throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);

throw ex;

Will make the change, thanks !!

adoroszlai · 2022-04-10T06:21:33Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java

      throw exception;
+    } catch (OS3Exception ex) {
+      getMetrics().incPutAclFailure();
+      LOG.debug("Failed to put acl of Bucket " + bucketName, ex);


We don't need the debug message, because the exception is logged at debug level in newError.

ozone/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java

Lines 141 to 145 in e81ab0a

if (e.getHttpCode() == HTTP_INTERNAL_ERROR) {

LOG.error("Internal Error: {}", err.toXml(), ex);

} else if (LOG.isDebugEnabled()) {

LOG.debug(err.toXml(), ex);

}

Suggested change

LOG.debug("Failed to put acl of Bucket " + bucketName, ex);

Will make the change, thanks !!

adoroszlai · 2022-04-10T06:22:10Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/metrics/S3GatewayMetrics.java


  // BucketEndpoint
  private @Metric MutableCounterLong getBucketSuccess;
+


Suggested change

Will make the change, thanks !!

adoroszlai · 2022-04-10T06:22:17Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/metrics/S3GatewayMetrics.java

  private @Metric MutableCounterLong getBucketSuccess;
+
  private @Metric MutableCounterLong getBucketFailure;
+


Suggested change

Will make the change, thanks !!

adoroszlai · 2022-04-10T06:33:10Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java

        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,


Nit: unrelated change.

Suggested change

LOG.debug("Error in Create Bucket Request for bucket: {}", bucketName,

LOG.error("Error in Create Bucket Request for bucket: {}", bucketName,

Will make the change, thanks !!

...p-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/metrics/TestS3GatewayMetrics.java

adoroszlai

Thanks @ArafatKhan2198 for updating the patch, especially for separating the test cases. I noticed one typo where the wrong metric is returned, otherwise it's looks good to me.

adoroszlai · 2022-04-11T19:34:56Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/metrics/S3GatewayMetrics.java

+  public long getListMultipartUploadsFailure() {
+    return listMultipartUploadsSuccess.value();


Sorry, I just noticed this typo:

Suggested change

public long getListMultipartUploadsFailure() {

return listMultipartUploadsSuccess.value();

public long getListMultipartUploadsFailure() {

return listMultipartUploadsFailure.value();

adoroszlai · 2022-04-11T19:35:23Z

...p-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/metrics/TestS3GatewayMetrics.java

+    InputStream inputBody = TestBucketAcl.class.getClassLoader()
+        .getResourceAsStream("userAccessControlList.xml");
+
+    try {
+      bucketEndpoint.put("unknown_bucket", ACL_MARKER, headers, inputBody);
+      fail();
+    } catch (OS3Exception ex) {
+    }
+    inputBody.close();


Nit: using try-with-resources we can ensure that the stream is closed even in case of failure.

Suggested change

InputStream inputBody = TestBucketAcl.class.getClassLoader()

.getResourceAsStream("userAccessControlList.xml");

try {

bucketEndpoint.put("unknown_bucket", ACL_MARKER, headers, inputBody);

fail();

} catch (OS3Exception ex) {

}

inputBody.close();

try (InputStream inputBody = TestBucketAcl.class.getClassLoader()

.getResourceAsStream("userAccessControlList.xml")) {

bucketEndpoint.put("unknown_bucket", ACL_MARKER, headers, inputBody);

fail();

} catch (OS3Exception ex) {

}

adoroszlai · 2022-04-11T19:35:39Z

...p-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/metrics/TestS3GatewayMetrics.java

+    // Failing the putACL endpoint by applying ACL on a non-Existent Bucket
+    long oriMetric = metrics.getPutAclFailure();
+
+    clientStub.getObjectStore().createS3Bucket("b1");


Nit: b1 bucket is not needed for this test case.

adoroszlai · 2022-04-11T19:36:23Z

...p-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/metrics/TestS3GatewayMetrics.java

+    InputStream inputBody = TestBucketAcl.class.getClassLoader()
+        .getResourceAsStream("userAccessControlList.xml");
+
+    bucketEndpoint.put("b1", ACL_MARKER, headers, inputBody);
+    inputBody.close();


Nit: using try-with-resources we can ensure that the stream is closed even in case of failure.

Suggested change

InputStream inputBody = TestBucketAcl.class.getClassLoader()

.getResourceAsStream("userAccessControlList.xml");

bucketEndpoint.put("b1", ACL_MARKER, headers, inputBody);

inputBody.close();

try (InputStream inputBody = TestBucketAcl.class.getClassLoader()

.getResourceAsStream("userAccessControlList.xml")) {

bucketEndpoint.put("b1", ACL_MARKER, headers, inputBody);

}

adoroszlai changed the title ~~Adding Unit-Test cases for S3-Gateway Bucket-Endpoint Metrics.~~ HDDS-6528. Adding Unit-Test cases for S3-Gateway Bucket-Endpoint Metrics. Apr 4, 2022

adoroszlai requested changes Apr 4, 2022

View reviewed changes

Mohammad Arafat Khan added 5 commits April 8, 2022 15:32

Adding Unit-Test cases for S3-Gateway Bucket-Endpoint Metrics.

864c706

Removed unused response variables

33f1e3d

Resolved the conflict

94148b1

Made changes as per asked

d24f75f

Made changes for code-review

e9787e1

ArafatKhan2198 force-pushed the HDDS-6528 branch from 6816ced to e9787e1 Compare April 8, 2022 10:20

ArafatKhan2198 requested a review from adoroszlai April 9, 2022 11:47

adoroszlai reviewed Apr 10, 2022

View reviewed changes

jojochuang reviewed Apr 11, 2022

View reviewed changes

...p-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/metrics/TestS3GatewayMetrics.java Show resolved Hide resolved

Made changes for code-review

8785de5

ArafatKhan2198 requested review from adoroszlai and jojochuang April 11, 2022 17:49

adoroszlai reviewed Apr 11, 2022

View reviewed changes

Made the necessary changes

4b9313d

ArafatKhan2198 requested a review from adoroszlai April 12, 2022 06:15

adoroszlai approved these changes Apr 12, 2022

View reviewed changes

jojochuang approved these changes Apr 12, 2022

View reviewed changes

jojochuang merged commit de0b1f9 into apache:master Apr 12, 2022

ArafatKhan2198 deleted the HDDS-6528 branch April 12, 2022 09:59

ArafatKhan2198 restored the HDDS-6528 branch April 12, 2022 18:57

		LOG.debug("Bucket Does not Exist " + bucketName, ex);
		throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);

	if (e.getHttpCode() == HTTP_INTERNAL_ERROR) {
	LOG.error("Internal Error: {}", err.toXml(), ex);
	} else if (LOG.isDebugEnabled()) {
	LOG.debug(err.toXml(), ex);
	}

	LOG.debug("Bucket Does not Exist " + bucketName, ex);
	throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);
	throw ex;

		LOG.debug("Failed to get acl of Bucket " + bucketName, ex);
		throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);


		// BucketEndpoint
		private @Metric MutableCounterLong getBucketSuccess;

	LOG.debug("Error in Create Bucket Request for bucket: {}", bucketName,
	LOG.error("Error in Create Bucket Request for bucket: {}", bucketName,

		public long getListMultipartUploadsFailure() {
		return listMultipartUploadsSuccess.value();

Conversation

ArafatKhan2198 commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArafatKhan2198 Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ArafatKhan2198 commented Apr 1, 2022 •

edited

Loading

ArafatKhan2198 Apr 6, 2022 •

edited

Loading