You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "swamirishi (via GitHub)" <gi...@apache.org> on 2023/07/26 08:18:50 UTC
[GitHub] [ozone] swamirishi opened a new pull request, #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
swamirishi opened a new pull request, #5119:
URL: https://github.com/apache/ozone/pull/5119
## What changes were proposed in this pull request?
OMFileRequest.getOMKeyInfoIfExists sets the keyname to complete keyname with prefix.
OMKeySetTimesRequestWithFSO calls this function and uses OmKeyInfo & doesn't revert keyName param back to leaf node name.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9082
## How was this patch tested?
Manual Testing on cluster while debugging.
--
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] jojochuang commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1275209178
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
Good catch! I wonder if we should fix OMFileRequest.getOMKeyInfoIfExists() instead because it's so confusing when does the keyName has the full prefix and when does not. I don't think we handle it consistently across the board.
I am fine with the stopgap fix now but i think we should revisit the handling of keyName and fileName.
--
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] swamirishi commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO should not prepend key prefix to keys in fileTable
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1275969827
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystemWithFSO.java:
##########
@@ -286,4 +292,35 @@ public void testLeaseRecoverable() throws Exception {
assertTrue(fs.recoverLease(source));
assertTrue(fs.isFileClosed(source));
}
+
+ @Test
Review Comment:
done
--
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] swamirishi commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1274973571
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
sure thanks a lot @sadanand48
--
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] swamirishi commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1275346121
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
I guess that has been done to keep it same as OBS & legacy buckets which has the full key name
--
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] hemantk-12 commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO should not prepend key prefix to keys in fileTable
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1275468691
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
> Adding a Helper function might not be helpful for other devs, having a unit test will have a failed CI.
If `OMFileRequest` is supposed to be use to access tables, then I think it is better to do that way. Otherwise as @jojochuang said, we should remove `keyName` update from `OMFileRequest.getOMKeyInfoIfExists()`.
In the end, I think we need to revisit the access of `keyTable` (may be `fileTable` and `dirTable` as well). If it is supposed to through `OMFileRequest`, then it should be used for both read and write.
--
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] umamaheswararao merged pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO should not prepend key prefix to keys in fileTable
Posted by "umamaheswararao (via GitHub)" <gi...@apache.org>.
umamaheswararao merged PR #5119:
URL: https://github.com/apache/ozone/pull/5119
--
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] swamirishi commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO should not prepend key prefix to keys in fileTable
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1276513035
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
We can do this refactoring as part of another 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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1275187645
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
> 1. Should we use helper function defined in `OMFileRequest` and used at other places. Similar to https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java#L508
@hemantk-12 I will work on another PR containing unit test to validate if the fileTable & DirTable cache has the correct value being set for all OMRequest. Adding a Helper function might not be helpful for other devs, having a unit test will have a failed CI.
--
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] hemantk-12 commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1275230059
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
+1 on @jojochuang. It is not a good idea to update the `keyName` in `OMFileRequest.getOMKeyInfoIfExists()`. Caller should take care of that.
--
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] sadanand48 commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1274811042
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
@swamirishi I added a unit test that I had written while trying. I hope its fine.
--
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] jojochuang commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1275345337
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystemWithFSO.java:
##########
@@ -286,4 +292,35 @@ public void testLeaseRecoverable() throws Exception {
assertTrue(fs.recoverLease(source));
assertTrue(fs.isFileClosed(source));
}
+
+ @Test
Review Comment:
This is fine. But you can also make the same unit test in TestOMKeySetTimesRequestWithFSO.java which is more lightweight (doesn't need to deal the entire cluster, client interactions just need to test OMKeySetTimesRequestWithFSO )
--
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] umamaheswararao commented on pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO should not prepend key prefix to keys in fileTable
Posted by "umamaheswararao (via GitHub)" <gi...@apache.org>.
umamaheswararao commented on PR #5119:
URL: https://github.com/apache/ozone/pull/5119#issuecomment-1654418801
Thanks @swamirishi for working on this. It is a good catch.
Thanks @hemantk-12, @jojochuang, @sadanand48 for the reviews
--
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] hemantk-12 commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1274591581
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
Good find.
1. Should we use helper function defined in `OMFileRequest` and used at other places. Similar to https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java#L508
2. Can you please add unit test for it?
--
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] swamirishi commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1275187645
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
> 1. Should we use helper function defined in `OMFileRequest` and used at other places. Similar to https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java#L508
@hemantk-12 I will work on another PR containing unit test to validate if the fileTable & DirTable cache has the correct value being set for all OMRequest. Adding a Helper function might not be helpful for other devs, having a unit test will have a failed CI.
--
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] hemantk-12 commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1274591581
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
Good find.
1. Should we use helper function defined in `OMFileRequest` and use at other places. Similar to https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java#L508
2. Can you please add unit test for it?
--
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] sadanand48 commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1274811042
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
I added a unit test that I had written while trying. I hope its fine.
--
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] swamirishi commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO adds key prefix to key in filetable
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1275187645
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
> 1. Should we use helper function defined in `OMFileRequest` and used at other places. Similar to https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java#L508
@hemantk-12 I will work on another PR containing unit test to validate if the fileTable & DirTable cache has the correct value being set for all OMRequest
--
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] hemantk-12 commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO should not prepend key prefix to keys in fileTable
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1275468691
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
> Adding a Helper function might not be helpful for other devs, having a unit test will have a failed CI.
If `OMFileRequest` is supposed to be use to access tables, then I think it is better to do that way. Otherwise as @jojochuang said, we should remove `keyName` update from `OMFileRequest.getOMKeyInfoIfExists()`.
In the end, I think we need to revisit the access of `keyTable` (may be `fileTable` and `dirTable` as well). It is supposed to through `OMFileRequest`, it should be used for both read and write.
--
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] hemantk-12 commented on a diff in pull request #5119: HDDS-9082. OMKeySetTimesRequestWithFSO should not prepend key prefix to keys in fileTable
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5119:
URL: https://github.com/apache/ozone/pull/5119#discussion_r1276581326
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequestWithFSO.java:
##########
@@ -96,6 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Key not found. Key:" + key, KEY_NOT_FOUND);
}
omKeyInfo = keyStatus.getKeyInfo();
+ // HDDS-setting Key name back to Ozone Key before updating cache value.
+ omKeyInfo.setKeyName(OzoneFSUtils.getFileName(key));
Review Comment:
Agree. It doesn't have to part of this PR. Please create a jira to revisit and may be refactor `OMFileRequest`.
--
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