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 2020/08/07 09:20:38 UTC
[GitHub] [hadoop-ozone] cxorm opened a new pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
cxorm opened a new pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301
## What changes were proposed in this pull request?
For renewing modification time when updating Acls of `Volume`/ `Bucket`/ `Key`, there are three parts to be modified in this patch.
**I. Volume Part**
- addacl: Add one or more new ACLs.
- removeacl: Remove one or more existing ACLs.
- setacl: Set one or more ACLs, replacing the existing ones.
**II. Bucket Part**
- addacl
- removeacl
- setacl
**III. Key Part**
- addacl
- removeacl
- setacl
#### Proposed fix.
Let `preExecute()` be overrided by `OMAddAclRequest`/ `OMSetAclRequest`/ `OMRemoveAclRequest` of `Volume`/ `Bucket`/`Key`, and evalute modification time in `preExecute()`.
After `preExecute()`, renew modification time after true `acl apply operation` in `validateAndUpdateCache()` of `OMVolumeAclRequest`/ `OMBucketAclRequest`/ `OMKeyAclRequest`.
**[note]**
In `OMVolumeAclRequest` and `OMBucketAclRequest`, the cache of table is updated only when true `acl apply operation` happening, so the code snippet of renewing modification time is added in this block.
But in `OMKeyAclRequest`, the original flow didn't consider boolean of `acl apply operation` when updating the cache of table, so the if-condition includes boolean of `acl apply operation`.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3882
## How was this patch tested?
- Modify/Add unit tests.
Volume Part : Just modify the `testPreExecute()` to verify modification time is renewed.
Bucket Part : Add new unit tests to verify `OMBucketAddAclRequest`/ `OMBucketRemoveAclRequest`/ `OMBucketSetAclRequest` including `testPreExecute()` and `testValidateAndUpdateCache`.
(Feel free to correct me if anyone think this part should be addressed in separated Jira or any thoughts :) )
Key Part : Modify original `TestOMKeyAclRequest.java` to `testKeyRemoveAclRequest()` and `testKeySetAclRequest()`.
(Again, Feel free to correct me if anyone think this part should be addressed in separated Jira or any thoughts, thanks)
- [Github action passed except codecov](https://github.com/cxorm/hadoop-ozone/runs/957353259)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301#discussion_r510486990
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAddAclRequest.java
##########
@@ -48,6 +50,19 @@
volumeAddAclOp = (acls, volArgs) -> volArgs.addAcl(acls.get(0));
}
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ long modificationTime = Time.now();
+ OzoneManagerProtocolProtos.AddAclRequest addAclRequest =
+ getOmRequest().getAddAclRequest().toBuilder()
Review comment:
Same as above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
cxorm commented on pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301#issuecomment-711871349
> Thanks @cxorm for the update. The latest PR LGTM overall, a few comments added inline.
Thank you @xiaoyuyao for the careful review.
The fix was added to addressed the comments.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
cxorm commented on pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301#issuecomment-670459094
Test would be added to pass code coverage.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301#issuecomment-716788128
Thanks @cxorm for fixing this. The change LGTM, +1. I will merge it shortly.
Also notice similar unnecessary builder conversion issue in OMVolumeSetQuotaRequestwhen the modify time is updated. Can you check and open separate JIRA to fix those? Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao merged pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
xiaoyuyao merged pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
cxorm commented on pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301#issuecomment-716102947
> Thanks for the update @cxorm. A few similar usage that can be optimized. Sorry I did not explicitly comment all of them last time.
That's my carelessness : )
Thanks a lot @xiaoyuyao for reviewing it again, the toBuilder issues in AclRequest has been addressed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301#discussion_r510486443
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/OMKeyAddAclRequest.java
##########
@@ -43,6 +45,19 @@
private static final Logger LOG =
LoggerFactory.getLogger(OMKeyAddAclRequest.class);
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ long modificationTime = Time.now();
+ OzoneManagerProtocolProtos.AddAclRequest addAclRequest =
+ getOmRequest().getAddAclRequest().toBuilder()
+ .setModificationTime(modificationTime).build();
Review comment:
This has the same toBuilder=>build=>toBuilder issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on a change in pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301#discussion_r511484216
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAclRequest.java
##########
@@ -84,6 +84,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
VOLUME_LOCK, volume);
omVolumeArgs = getVolumeInfo(omMetadataManager, volume);
+
Review comment:
nit
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on a change in pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301#discussion_r511484216
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAclRequest.java
##########
@@ -84,6 +84,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
VOLUME_LOCK, volume);
omVolumeArgs = getVolumeInfo(omMetadataManager, volume);
+
Review comment:
nit: redundant line
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm edited a comment on pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
cxorm edited a comment on pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301#issuecomment-711871349
> Thanks @cxorm for the update. The latest PR LGTM overall, a few comments added inline.
Thank you @xiaoyuyao for the careful review.
The fix was added to address the comments.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301#discussion_r510486787
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/OMKeySetAclRequest.java
##########
@@ -44,6 +46,19 @@
private static final Logger LOG =
LoggerFactory.getLogger(OMKeySetAclRequest.class);
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ long modificationTime = Time.now();
+ OzoneManagerProtocolProtos.SetAclRequest setAclRequest =
+ getOmRequest().getSetAclRequest().toBuilder()
Review comment:
Same as above. Sorry, I did not explicitly comment all on this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301#discussion_r510486544
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/OMKeyRemoveAclRequest.java
##########
@@ -43,6 +45,19 @@
private static final Logger LOG =
LoggerFactory.getLogger(OMKeyRemoveAclRequest.class);
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ long modificationTime = Time.now();
+ OzoneManagerProtocolProtos.RemoveAclRequest removeAclRequest =
+ getOmRequest().getRemoveAclRequest().toBuilder()
+ .setModificationTime(modificationTime).build();
Review comment:
Same as above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1301: HDDS-3882. Update modification time when updating volume/bucket/key ACLs
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1301:
URL: https://github.com/apache/hadoop-ozone/pull/1301#discussion_r503654340
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -58,6 +60,19 @@
};
}
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ long modificationTime = Time.now();
+ OzoneManagerProtocolProtos.AddAclRequest addAclRequest =
+ getOmRequest().getAddAclRequest().toBuilder()
+ .setModificationTime(modificationTime).build();
Review comment:
if you don't call .build() hereon line 68, we can avoid the unnecessary toBuilder() conversion on line 71.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketRemoveAclRequest.java
##########
@@ -55,6 +57,19 @@
};
}
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ long modificationTime = Time.now();
+ OzoneManagerProtocolProtos.RemoveAclRequest removeAclRequest =
+ getOmRequest().getRemoveAclRequest().toBuilder()
Review comment:
same as above.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketSetAclRequest.java
##########
@@ -55,6 +57,19 @@
};
}
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ long modificationTime = Time.now();
+ OzoneManagerProtocolProtos.SetAclRequest setAclRequest =
+ getOmRequest().getSetAclRequest().toBuilder()
Review comment:
same as above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org