-
Notifications
You must be signed in to change notification settings - Fork 475
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 #1192
Conversation
@@ -1016,6 +1016,7 @@ public OzoneFileStatus getOzoneFileStatus(String volumeName, | |||
.setBucketName(bucketName) | |||
.setKeyName(keyName) | |||
.setRefreshPipeline(true) | |||
.setSortDatanodesInPipeline(topologyAwareReadEnabled) |
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.
@rakeshadr , can you plz add some details here as to why sorting of dns is required ?
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 @bshashikant for the reviews. This change is basically to make "getFileStatus" and "listStatus" apis similar to "lookupFile()" API, where it does sorting.
Reference:
#1164 (comment)
/pending "@rakeshadr , can you plz add some details here as to why sorting of dns is required ?" |
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.
Marking this issue as un-mergeable as requested.
Please use /ready
comment when it's resolved.
"@rakeshadr , can you plz add some details here as to why sorting of dns is required ?"
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. Can you please update the patch to resolve conflicts?
* @return file status. | ||
* @throws IOException | ||
*/ | ||
OzoneFileStatus getFileStatus(OmKeyArgs args, 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.
Should we also keep the two original methods? Even if backwards compatibility is not required for this interface, it would let us avoid changing lots of calls just to pass null
.
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.
Agreed, will keep the existing #getFileStatus(args) method.
* @param clientAddress a hint to key manager, order the datanode in returned | ||
* pipeline by distance between client and datanode. | ||
* @return file status. | ||
* @throws IOException |
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.
Thank you for adding updated doc from KeyManagerImpl
to the interface. Is there a reason exception is not described (the cases when they are thrown)? Also, can you please remove the old javadoc from the implementation?
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.
No specific reason, I will describe exception cases.
I will update the KeyManagerImpl implementation as well.
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 for the reviews
* @param clientAddress a hint to key manager, order the datanode in returned | ||
* pipeline by distance between client and datanode. | ||
* @return file status. | ||
* @throws IOException |
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.
No specific reason, I will describe exception cases.
I will update the KeyManagerImpl implementation as well.
* @return file status. | ||
* @throws IOException | ||
*/ | ||
OzoneFileStatus getFileStatus(OmKeyArgs args, 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.
Agreed, will keep the existing #getFileStatus(args) method.
…her than rely on proto serialization for key. (apache#1197)
…nodes in the pipeline. (apache#1154)
…es used if they differ in SCM (apache#1339)
Co-authored-by: lamberken <xielei@cmss.chinamobile.com>
…tus #listStatus APIs
I am afraid something wrong happened with the last rebase/merge:
Seems that you rebased the master to your branch instead of rebasing your branch to the master. The easiest way to fix can be the re-creation of the branch, cherry-picking required commits and doing force push. |
yes, I polluted the PR. Thanks @elek for the suggestion. I have created another branch. @bshashikant , @adoroszlai I have created fresh PR #1385 as this PR has been polluted:-) I have addressed review comments in that new PR. |
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
How was this patch tested?
Used existing TestKeyManagerImpl unit test cases.