New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-3947: Sort DNs for client when the key is a file for #getFileStatus #listStatus APIs #1385
Conversation
…tus #listStatus APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rakeshadr for updating the patch.
@@ -1783,7 +1802,9 @@ private OzoneFileStatus getOzoneFileStatus(String volumeName, | |||
|
|||
// if the key is a file then do refresh pipeline info in OM by asking SCM | |||
if (fileKeyInfo != null) { | |||
refreshPipeline(fileKeyInfo); | |||
if (refreshPipeline) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think refreshPipeline
condition was removed intentionally in HDDS-3658 and should not be restored. (This is also causing failure of TestKeyManagerUnit.testLookupFileWithDnFailure
unit test.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @adoroszlai again for the review help.
Yes, you are correct. Test passed locally without the if (refreshPipeline)
condition.
Point-1) That means, the caller should callrefreshPipeline(OmKeyInfo value)
method without the refreshPipeline
condition.
Point-2) Also, I'm planning to move the refreshPipeline call outside BUCKET_LOCK and will be moving to here. Now, its a duplicate call added as part of HDDS-3658.
Does these two changes make sense to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, both points make sense. Thanks for noticing the duplicate call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, Done!
for(OzoneFileStatus fileStatus : fileStatusList){ | ||
|
||
for (OzoneFileStatus fileStatus : fileStatusList) { | ||
if (args.getRefreshPipeline()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChenSammi Was this instance of getRefreshPipeline
condition retained in HDDS-3658 intentionally?
List<OzoneFileStatus> listStatus(OmKeyArgs keyArgs, boolean recursive, | ||
String startKey, long numEntries) throws IOException; | ||
String startKey, long numEntries, String clientAddress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my comment on the previous PR:
Should we also keep the two original methods?
I meant both getFileStatus
and listStatus
. Sorry if it was unclear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will make the changes in my next commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adoroszlai , Sorry for re-opening the discussion on this comment. On a second thought listStatus
is only used in one place in the source code and there won't be any other caller for listStatus() without ClientAddress
. So, how about deleting the original method and can add it later if there is any need ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listStatus is only used in one place in the source code
It is also used by several tests, which all pass null
as address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, addressed in my new commit
…tatus overloaded api
Thanks @adoroszlai for the reviews. Uploaded new commit addressing your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rakeshadr for working on this. The patch LGTM, +1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rakeshadr for updating the patch.
I have one question left: shouldn't getFileStatus
call from listStatus
pass clientAddress
?
Good catch. Thanks @adoroszlai. Updated new commit. I've added clientAddress to the #getFileStatus call. Also, I've moved #getFileStatus call outside BUCKET_LOCK so that it will get the benefit of doing 'sort' and 'refreshPipeline' outside BUCKET_LOCK. |
Thanks a lot @xiaoyuyao for the reviews! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rakeshadr for improving this patch several times.
Thanks @rakeshadr for the contribution, and @xiaoyuyao for the review. |
Thanks a lot @adoroszlai for review help and merging it. |
* master: HDDS-4102. Normalize Keypath for lookupKey. (apache#1328) HDDS-4263. ReplicatiomManager shouldn't consider origin node Id for CLOSED containers. (apache#1438) HDDS-4282. Improve the emptyDir syntax (apache#1450) HDDS-4194. Create a script to check AWS S3 compatibility (apache#1383) HDDS-4270. Add more reusable byteman scripts to debug ofs/o3fs performance (apache#1443) HDDS-2660. Create insight point for datanode container protocol (apache#1272) HDDS-3297. Enable TestOzoneClientKeyGenerator. (apache#1442) HDDS-4324. Add important comment to ListVolumes logic (apache#1417) HDDS-4236. Move "Om*Codec.java" to new project hadoop-ozone/interface-storage (apache#1424) HDDS-4254. Bucket space: add usedBytes and update it when create and delete key. (apache#1431) HDDS-2766. security/SecuringDataNodes.md (apache#1175) HDDS-4206. Attempt pipeline creation more frequently in acceptance tests (apache#1389) HDDS-4233. Interrupted exeception printed out from DatanodeStateMachine (apache#1416) HDDS-3947: Sort DNs for client when the key is a file for #getFileStatus #listStatus APIs (apache#1385) HDDS-3102. ozone getconf command should use the GenericCli parent class (apache#1410) HDDS-3981. Add more debug level log to XceiverClientGrpc for debug purpose (apache#1214) HDDS-4255. Remove unused Ant and Jdiff dependency versions (apache#1433) HDDS-4247. Fixed log4j usage in some places (apache#1426) HDDS-4241. Support HADOOP_TOKEN_FILE_LOCATION for Ozone token CLI. (apache#1422)
* HDDS-4122-remove-code-consolidation: (21 commits) Restore files that had deduplicated code from master Revert other delete request/response files back to their original states on master HDDS-4102. Normalize Keypath for lookupKey. (apache#1328) HDDS-4263. ReplicatiomManager shouldn't consider origin node Id for CLOSED containers. (apache#1438) HDDS-4282. Improve the emptyDir syntax (apache#1450) HDDS-4194. Create a script to check AWS S3 compatibility (apache#1383) HDDS-4270. Add more reusable byteman scripts to debug ofs/o3fs performance (apache#1443) HDDS-2660. Create insight point for datanode container protocol (apache#1272) HDDS-3297. Enable TestOzoneClientKeyGenerator. (apache#1442) HDDS-4324. Add important comment to ListVolumes logic (apache#1417) HDDS-4236. Move "Om*Codec.java" to new project hadoop-ozone/interface-storage (apache#1424) HDDS-4254. Bucket space: add usedBytes and update it when create and delete key. (apache#1431) HDDS-2766. security/SecuringDataNodes.md (apache#1175) HDDS-4206. Attempt pipeline creation more frequently in acceptance tests (apache#1389) HDDS-4233. Interrupted exeception printed out from DatanodeStateMachine (apache#1416) HDDS-3947: Sort DNs for client when the key is a file for #getFileStatus #listStatus APIs (apache#1385) HDDS-3102. ozone getconf command should use the GenericCli parent class (apache#1410) HDDS-3981. Add more debug level log to XceiverClientGrpc for debug purpose (apache#1214) HDDS-4255. Remove unused Ant and Jdiff dependency versions (apache#1433) HDDS-4247. Fixed log4j usage in some places (apache#1426) ...
What changes were proposed in this pull request?
Similar to OzoneManagerFS#lookupFile(OmKeyArgs args, String clientAddress) interface, "clientAddress" has been passed as an argument to OzoneManagerFS#listStatus(), OzoneManagerFS#getFileStatus() APIs.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3947
Please replace this section with the link to the Apache JIRA)
How was this patch tested?
Used existing TestKeyManagerImpl unit test cases.