You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/01/13 21:24:04 UTC
[GitHub] [ozone] aswinshakil opened a new pull request #2988: HDDS-6171. Create an API to change Bucket Owner
aswinshakil opened a new pull request #2988:
URL: https://github.com/apache/ozone/pull/2988
## What changes were proposed in this pull request?
Right now, the bucket owner is set when the bucket is created. We need an API to change the owner of a bucket after the bucket is created.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6171
## How was this patch tested?
This patch was tested manually and using integration tests.
--
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
[GitHub] [ozone] smengcl merged pull request #2988: HDDS-6171. Create an API to change Bucket Owner
Posted by GitBox <gi...@apache.org>.
smengcl merged pull request #2988:
URL: https://github.com/apache/ozone/pull/2988
--
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
[GitHub] [ozone] aswinshakil commented on a change in pull request #2988: HDDS-6171. Create an API to change Bucket Owner
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on a change in pull request #2988:
URL: https://github.com/apache/ozone/pull/2988#discussion_r786996603
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java
##########
@@ -297,6 +297,26 @@ public void testVolumeSetOwner() throws IOException {
proxy.setVolumeOwner(volumeName, ownerName);
}
+ @Test
+ public void testBucketSetOwner() throws IOException {
+ String volumeName = UUID.randomUUID().toString();
+ String bucketName = UUID.randomUUID().toString();
+ store.createVolume(volumeName);
+ store.getVolume(volumeName).createBucket(bucketName);
+
+ String oldOwner = store.getVolume(volumeName).getBucket(bucketName)
+ .getOwner();
+ String ownerName = "testUser";
+
+ ClientProtocol proxy = store.getClientProxy();
+ proxy.setBucketOwner(volumeName, bucketName, ownerName);
+ String newOwner = store.getVolume(volumeName).getBucket(bucketName)
+ .getOwner();
+
+ assertEquals(ownerName, newOwner);
+ assertNotEquals(oldOwner, newOwner);
+ }
Review comment:
I looked at the other tests in `TestOzoneRpcClientAbstract`. It doesn't have a clean-up logic as well. Wouldn't it clean when we shutdown the cluster?
--
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
[GitHub] [ozone] smengcl commented on a change in pull request #2988: HDDS-6171. Create an API to change Bucket Owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2988:
URL: https://github.com/apache/ozone/pull/2988#discussion_r786901358
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
##########
@@ -772,4 +772,14 @@ OzoneKey headObject(String volumeName, String bucketName,
* Clears the S3 Authentication information attached to the thread.
*/
void clearTheadLocalS3Auth();
+
+ /**
+ * Sets the owner of bucket.
+ * @param volumeName Name of the Volume
+ * @param bucketName Name of the Bucket
+ * @param owner to be set for the bucket
+ * @throws IOException
+ */
+ void setBucketOwner(String volumeName, String bucketName,
Review comment:
Let's follow `setVolumeOwner()` and return `boolean` instead?
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java
##########
@@ -297,6 +297,26 @@ public void testVolumeSetOwner() throws IOException {
proxy.setVolumeOwner(volumeName, ownerName);
}
+ @Test
+ public void testBucketSetOwner() throws IOException {
+ String volumeName = UUID.randomUUID().toString();
+ String bucketName = UUID.randomUUID().toString();
+ store.createVolume(volumeName);
+ store.getVolume(volumeName).createBucket(bucketName);
+
+ String oldOwner = store.getVolume(volumeName).getBucket(bucketName)
+ .getOwner();
+ String ownerName = "testUser";
+
+ ClientProtocol proxy = store.getClientProxy();
+ proxy.setBucketOwner(volumeName, bucketName, ownerName);
+ String newOwner = store.getVolume(volumeName).getBucket(bucketName)
+ .getOwner();
+
+ assertEquals(ownerName, newOwner);
+ assertNotEquals(oldOwner, newOwner);
+ }
Review comment:
Would you add a clean up logic to this test case? i.e. delete the bucket and volume created in this test case at the end.
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java
##########
@@ -230,6 +250,9 @@ public BucketArgs getProtobuf() {
if(quotaInNamespace > 0 || quotaInNamespace == OzoneConsts.QUOTA_RESET) {
builder.setQuotaInNamespace(quotaInNamespace);
}
+ if(ownerName != null) {
Review comment:
```suggestion
if (ownerName != null) {
```
--
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
[GitHub] [ozone] aswinshakil commented on a change in pull request #2988: HDDS-6171. Create an API to change Bucket Owner
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on a change in pull request #2988:
URL: https://github.com/apache/ozone/pull/2988#discussion_r787023173
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java
##########
@@ -297,6 +297,26 @@ public void testVolumeSetOwner() throws IOException {
proxy.setVolumeOwner(volumeName, ownerName);
}
+ @Test
+ public void testBucketSetOwner() throws IOException {
+ String volumeName = UUID.randomUUID().toString();
+ String bucketName = UUID.randomUUID().toString();
+ store.createVolume(volumeName);
+ store.getVolume(volumeName).createBucket(bucketName);
+
+ String oldOwner = store.getVolume(volumeName).getBucket(bucketName)
+ .getOwner();
+ String ownerName = "testUser";
+
+ ClientProtocol proxy = store.getClientProxy();
+ proxy.setBucketOwner(volumeName, bucketName, ownerName);
+ String newOwner = store.getVolume(volumeName).getBucket(bucketName)
+ .getOwner();
+
+ assertEquals(ownerName, newOwner);
+ assertNotEquals(oldOwner, newOwner);
+ }
Review comment:
Thank you for the explanation. I will update the PR with the suggestions
--
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
[GitHub] [ozone] aswinshakil commented on a change in pull request #2988: HDDS-6171. Create an API to change Bucket Owner
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on a change in pull request #2988:
URL: https://github.com/apache/ozone/pull/2988#discussion_r787007152
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
##########
@@ -772,4 +772,14 @@ OzoneKey headObject(String volumeName, String bucketName,
* Clears the S3 Authentication information attached to the thread.
*/
void clearTheadLocalS3Auth();
+
+ /**
+ * Sets the owner of bucket.
+ * @param volumeName Name of the Volume
+ * @param bucketName Name of the Bucket
+ * @param owner to be set for the bucket
+ * @throws IOException
+ */
+ void setBucketOwner(String volumeName, String bucketName,
Review comment:
Yes I can add a response for `SetBucketPropertyResponse` to return boolean in `OmClientProtocol.proto`
--
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
[GitHub] [ozone] smengcl commented on a change in pull request #2988: HDDS-6171. Create an API to change Bucket Owner
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2988:
URL: https://github.com/apache/ozone/pull/2988#discussion_r787018187
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java
##########
@@ -297,6 +297,26 @@ public void testVolumeSetOwner() throws IOException {
proxy.setVolumeOwner(volumeName, ownerName);
}
+ @Test
+ public void testBucketSetOwner() throws IOException {
+ String volumeName = UUID.randomUUID().toString();
+ String bucketName = UUID.randomUUID().toString();
+ store.createVolume(volumeName);
+ store.getVolume(volumeName).createBucket(bucketName);
+
+ String oldOwner = store.getVolume(volumeName).getBucket(bucketName)
+ .getOwner();
+ String ownerName = "testUser";
+
+ ClientProtocol proxy = store.getClientProxy();
+ proxy.setBucketOwner(volumeName, bucketName, ownerName);
+ String newOwner = store.getVolume(volumeName).getBucket(bucketName)
+ .getOwner();
+
+ assertEquals(ownerName, newOwner);
+ assertNotEquals(oldOwner, newOwner);
+ }
Review comment:
Yes it is unfortunate that not all UTs clean up after themselves.
ofc when the **whole** test class shuts down everything should be wiped. But in-between test cases in the same test class, it is often not. Test cases in the same (integration) test class usually share the same cluster to speed up the test suite.
A lot of the times not cleaning up interferes with some test cases (e.g. ones that checks for number of volumes or expects a clean cluster), which doesn't play nicely with newly contributed test cases later. It is generally a good practice to do the clean up.
Let's not bother with other test cases in this class for now. Appending the two lines to your new test case would suffice for this PR.
--
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