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/02/16 21:20:56 UTC
[GitHub] [ozone] adoroszlai opened a new pull request #3103: HDDS-6321. Avoid refresh pipeline for key lookup in checkAcls
adoroszlai opened a new pull request #3103:
URL: https://github.com/apache/ozone/pull/3103
## What changes were proposed in this pull request?
Reduce RPC calls by avoiding content-related operations (refresh pipeline, etc.) for ACL check. Reuse `isHeadOp` flag for this purpose.
https://issues.apache.org/jira/browse/HDDS-6321
## How was this patch tested?
Reproduced manually on `master` in `ozonesecure` environment.
Added integration test:
```
[ERROR] Tests run: 20, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 16.828 s <<< FAILURE! - in org.apache.hadoop.ozone.om.TestKeyManagerImpl
[ERROR] testCheckAccessForFileKey Time elapsed: 0.044 s <<< FAILURE!
org.mockito.exceptions.verification.NeverWantedButInvoked:
storageContainerLocationProtocol.getContainerWithPipelineBatch(
<any>
);
Never wanted here:
-> at org.apache.hadoop.ozone.om.TestKeyManagerImpl.testCheckAccessForFileKey(TestKeyManagerImpl.java:486)
But invoked here:
-> at org.apache.hadoop.ozone.om.KeyManagerImpl.refreshPipeline(KeyManagerImpl.java:507)
```
and verified the change:
```
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 15.019 s - in org.apache.hadoop.ozone.om.TestKeyManagerImpl
```
https://github.com/adoroszlai/hadoop-ozone/runs/5220817724#step:4:2886
--
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] adoroszlai commented on pull request #3103: HDDS-6321. Avoid refresh pipeline for key lookup in checkAcls
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #3103:
URL: https://github.com/apache/ozone/pull/3103#issuecomment-1048995278
Thanks @sodonnel for the review.
> How does this change impact TrashOzoneFileSystem and OzonePrefixPathImpl as `setHeadOp` is set to true there too?
`OzonePrefixPathImpl` is used for prefix ACLs.
You are right, `TrashOzoneFileSystem` has nothing to do with ACL check. I looked for similar usage of `getFileStatus` / `listStatus` where the result's location information is unused. This class converts the return value to Hadoop's `FileStatus` ignoring location:
https://github.com/apache/ozone/blob/f0ee98a45d5371fa9595f8178eca72b1bfe37e2b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashOzoneFileSystem.java#L232-L250
--
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] sodonnel commented on pull request #3103: HDDS-6321. Avoid refresh pipeline for key lookup in checkAcls
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #3103:
URL: https://github.com/apache/ozone/pull/3103#issuecomment-1048940846
If we don't refresh the locations, does the returned KeyInfo still have locations that were stored in OM, but they may be stale? I guess the change here is for checkAccess, where `.setHeadOp(true)` is set to true now, and it is more of an internal call, so it probably doesn't matter.
How does this change impact TrashOzoneFileSystem and OzonePrefixPathImpl as `setHeadOp` is set to true there too?. I guess I am not too familiar with the OM code, so I am just trying to understand how this change fits into improving the ACL checks.
--
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] adoroszlai commented on pull request #3103: HDDS-6321. Avoid refresh pipeline for key lookup in checkAcls
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #3103:
URL: https://github.com/apache/ozone/pull/3103#issuecomment-1042322758
@kuenishi Thanks for reporting the problem. Please review if you have some time.
--
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] sodonnel commented on pull request #3103: HDDS-6321. Avoid refresh pipeline for key lookup in checkAcls
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #3103:
URL: https://github.com/apache/ozone/pull/3103#issuecomment-1049034006
OK - makes sense. This change makes sense to me, and I am 95% sure it is OK to commit. However with my lack of familiarity with the OM code, I think we should get @rakeshadr to have a quick look too.
--
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] rakeshadr commented on a change in pull request #3103: HDDS-6321. Avoid refresh pipeline for key lookup in checkAcls
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #3103:
URL: https://github.com/apache/ozone/pull/3103#discussion_r817871286
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1244,15 +1240,19 @@ private OzoneFileStatus getOzoneFileStatus(String volumeName,
// if the key is a file then do refresh pipeline info in OM by asking SCM
if (fileKeyInfo != null) {
- if (latestLocationVersion) {
- slimLocationVersion(fileKeyInfo);
- }
- // refreshPipeline flag check has been removed as part of
- // https://issues.apache.org/jira/browse/HDDS-3658.
- // Please refer this jira for more details.
- refresh(fileKeyInfo);
- if (sortDatanodes) {
- sortDatanodes(clientAddress, fileKeyInfo);
+ // If operation is head, do not perform any additional steps
+ // As head operation does not need any of those details.
+ if (!args.isHeadOp()) {
+ if (args.getLatestVersionLocation()) {
Review comment:
I think, need to move latest version location outside `if (!args.isHeadOp())` block. Please make the changes for all the occurrences of `args.getLatestVersionLocation()` condition.
```
if (args.getLatestVersionLocation()) {
slimLocationVersion(fileKeyInfo);
}
```
--
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] adoroszlai merged pull request #3103: HDDS-6321. Avoid refresh pipeline for key lookup in checkAcls
Posted by GitBox <gi...@apache.org>.
adoroszlai merged pull request #3103:
URL: https://github.com/apache/ozone/pull/3103
--
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] adoroszlai commented on pull request #3103: HDDS-6321. Avoid refresh pipeline for key lookup in checkAcls
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #3103:
URL: https://github.com/apache/ozone/pull/3103#issuecomment-1058028873
Thanks @kuenishi, @rakeshadr, @sodonnel for the review.
--
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] rakeshadr commented on a change in pull request #3103: HDDS-6321. Avoid refresh pipeline for key lookup in checkAcls
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #3103:
URL: https://github.com/apache/ozone/pull/3103#discussion_r818242489
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1294,17 +1295,17 @@ private OzoneFileStatus getOzoneFileStatusFSO(String volumeName,
if (fileStatus != null) {
// if the key is a file then do refresh pipeline info in OM by asking SCM
- if (fileStatus.isFile()) {
+ if (fileStatus.isFile() && !args.isHeadOp()) {
OmKeyInfo fileKeyInfo = fileStatus.getKeyInfo();
- if (latestLocationVersion) {
+ if (args.getLatestVersionLocation()) {
Review comment:
@adoroszlai, same here as well. Could you please keep only refresh() and sortDatanodes() inside `if (!args.isHeadOp())` block and move LatestVersionLocation outside.
--
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] rakeshadr commented on a change in pull request #3103: HDDS-6321. Avoid refresh pipeline for key lookup in checkAcls
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #3103:
URL: https://github.com/apache/ozone/pull/3103#discussion_r817871286
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1244,15 +1240,19 @@ private OzoneFileStatus getOzoneFileStatus(String volumeName,
// if the key is a file then do refresh pipeline info in OM by asking SCM
if (fileKeyInfo != null) {
- if (latestLocationVersion) {
- slimLocationVersion(fileKeyInfo);
- }
- // refreshPipeline flag check has been removed as part of
- // https://issues.apache.org/jira/browse/HDDS-3658.
- // Please refer this jira for more details.
- refresh(fileKeyInfo);
- if (sortDatanodes) {
- sortDatanodes(clientAddress, fileKeyInfo);
+ // If operation is head, do not perform any additional steps
+ // As head operation does not need any of those details.
+ if (!args.isHeadOp()) {
+ if (args.getLatestVersionLocation()) {
Review comment:
Thanks @adoroszlai for taking care this case.
I think, need to move latest version location outside `if (!args.isHeadOp())` block.
```
if (args.getLatestVersionLocation()) {
slimLocationVersion(fileKeyInfo);
}
```
--
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] adoroszlai commented on a change in pull request #3103: HDDS-6321. Avoid refresh pipeline for key lookup in checkAcls
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3103:
URL: https://github.com/apache/ozone/pull/3103#discussion_r817890899
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1244,15 +1240,19 @@ private OzoneFileStatus getOzoneFileStatus(String volumeName,
// if the key is a file then do refresh pipeline info in OM by asking SCM
if (fileKeyInfo != null) {
- if (latestLocationVersion) {
- slimLocationVersion(fileKeyInfo);
- }
- // refreshPipeline flag check has been removed as part of
- // https://issues.apache.org/jira/browse/HDDS-3658.
- // Please refer this jira for more details.
- refresh(fileKeyInfo);
- if (sortDatanodes) {
- sortDatanodes(clientAddress, fileKeyInfo);
+ // If operation is head, do not perform any additional steps
+ // As head operation does not need any of those details.
+ if (!args.isHeadOp()) {
+ if (args.getLatestVersionLocation()) {
Review comment:
Thanks @rakeshadr for the review. I have updated 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.
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