-
Notifications
You must be signed in to change notification settings - Fork 490
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-4357: Rename : make rename an atomic ops by updating key path entry in dir/file table #1528
Conversation
…le entry into separate FileTable (apache#1473)
…try in dir/file table
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.
Haven't taken a deep review, catch one place:
if (fromKeyValue.getKeyName().equals(toKeyValue.getKeyName())) { | ||
// case-1) src == destin then check source and destin of same type | ||
// If dst is a file then return true. Otherwise fail the operation. | ||
if (!toKeyFileStatus.isDirectory()) { |
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 directly throw error here no matter toKeyFileStatus is a directory or file. I see in KeyManagerImpl#renameKey, it just returns.
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've referred following code in BasicOzoneFileSystem.java#L369 and IIUC, it is returning false for a directory and true for a file.
Appreciate if you could point me to the code you are referring to understand the case well. Thanks!
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 was looking at these lines before, KeyManagerImpl.java#L821
I'm okay to keep this same with BasicOzoneFileSystem logic did.
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 for the confirmation. As I said, I have created new PR #1557. It is more or less same logic and done following things:
- Addressed your comments
- Resolved merge conflicts
Could you please review the changes when you get a chance. Thanks!
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, have taken the review, : ).
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 again for the great help:-)
OzoneFileStatus toKeyParentDirStatus = getOMKeyInfoIfExists(metaMgr, | ||
volumeName, bucketName, toKeyParentDir, 0); | ||
// check if the immediate parent exists | ||
if (toKeyParentDirStatus == 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.
Can we add a safe check toKeyParentDirStatus.isDirectory()to make sure toKeyParentDirStatus is a directory
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 do it in new 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.
Sure, I will do it in new commit
... Please go through my reply.
I didn't see your reply, do you commit the comments?
@rakeshadr , current latest commit include many previous commits? Would you update with the latest commit as HDDS-4332 already merged.
oops, sorry for that. I had added my comment and shown in my UI as duplicate then I have deleted it. That caused the issue and my entire comments got wiped out:-)
I have added back my comments. Kindly go through it again. Thanks for your time.
I am planning to create new PR as merging takes more time, by addressing all your comments apart from one comment
} else if (toKeyFileStatus.isDirectory()) { | ||
// case-2) If dst is a directory, rename source as sub-path of it. | ||
// For example: rename /source to /dst will lead to /dst/source | ||
String fromFileName = OzoneFSUtils.getFileName(fromKeyName); |
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.
Rename to its subdirectory is also not allowed when subdirectory is existed.
Example,
/a
/a/subdir
Move /a to /a/subdir is an invalid operation.
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.
Good catch. I will do this check.
181bb34
to
f707bba
Compare
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 @linyiqun for the review comments. Please go through my reply.
I didn't see your reply, do you commit the comments? @rakeshadr , current latest commit include many previous commits? Would you update with the latest commit as HDDS-4332 already merged. |
Opened new PR #1557. Will close this current PR in few days. Just keeping as a reference now. |
What changes were proposed in this pull request?
https://issues.apache.org/jira/browse/HDDS-4357
What is the link to the Apache JIRA
This task is to handle rename key path request and make it an atomic operation by updating the DirTable or FileTable.
How was this patch tested?
Added TestOzoneFileSystem UT.
Note: Few refactoring has to be done once HDDS-4332 getFileStatus/ListStatus API is committed to the branch.