[
https://issues.apache.org/jira/browse/HDFS-6562?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14039444#comment-14039444
]
Jing Zhao edited comment on HDFS-6562 at 6/20/14 10:11 PM:
-----------------------------------------------------------
Thanks for the refactoring, Haohui! Just finished reviewing the changes for the
old rename. Some comments:
# Maybe we can let checkSnapshot throw a SnapshotException instead of the
current general IOException. In this way for the old version rename, we do not
need to check the exception msg when validating the rename source.
# Maybe we can rename RenameTransaction to RenameOperation?
# In RenameTransaction, the initialization of srcChildName, isSrcInSnapshot, and
scrChildIsReference can be moved into the constructor so that they can still
be declared as final.
# Since we no longer have INode replacement, we do not need to replace the last
inode of the srcIIP. Simply calling srcChild.recordModification should be good
enough.
{code}
if (isSrcInSnapshot) {
srcChild = srcChild.recordModification(srcIIP.getLatestSnapshotId());
srcIIP.setLastINode(srcChild);
}
{code}
# Similarly I guess we do not need the following code any more (this is in the
new rename):
{code}
if (dstParent.getParent() == null) {
// src and dst file/dir are in the same directory, and the dstParent has
// been replaced when we removed the src. Refresh the dstIIP and
// dstParent.
dstIIP = getINodesInPath4Write(dst, false);
}
{code}
And in removeLastINode(INodesInPath), we can remove the following code:
{code}
INodeDirectory newParent = last.getParent();
if (parent != newParent) {
iip.setINode(-2, newParent);
}
{code}
Will continue reviewing the new rename.
was (Author: jingzhao):
Thanks for the refactoring, Haohui! Just finished reviewing the changes for the
old rename. Some comments:
# Maybe we can let checkSnapshot throw a SnapshotException instead of the
current general IOException. In this way for the old version rename, we do not
need to check the exception msg when validating the rename source.
# Maybe we can rename RenameTransaction to RenameOperation?
# In RenameTransaction, the initialization of srcChildName, isSrcInSnapshot, and
scrChildIsReference can be moved into the constructor so that they can still
be declared as final.
# Since we no longer have INode replacement, we do not need to replace the last
inode of the srcIIP. Simply calling srcChild.recordModification should be good
enough.
{code}
if (isSrcInSnapshot) {
srcChild = srcChild.recordModification(srcIIP.getLatestSnapshotId());
srcIIP.setLastINode(srcChild);
}
{code}
# Similarly I guess we do not need the following code any more:
{code}
if (dstParent.getParent() == null) {
// src and dst file/dir are in the same directory, and the dstParent has
// been replaced when we removed the src. Refresh the dstIIP and
// dstParent.
dstIIP = getINodesInPath4Write(dst, false);
}
{code}
And in removeLastINode(INodesInPath), we can remove the following code:
{code}
INodeDirectory newParent = last.getParent();
if (parent != newParent) {
iip.setINode(-2, newParent);
}
{code}
Will continue reviewing the new rename.
> Refactor rename() in FSDirectory
> --------------------------------
>
> Key: HDFS-6562
> URL: https://issues.apache.org/jira/browse/HDFS-6562
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: namenode
> Reporter: Haohui Mai
> Assignee: Haohui Mai
> Attachments: HDFS-6562.000.patch, HDFS-6562.001.patch,
> HDFS-6562.002.patch, HDFS-6562.003.patch
>
>
> Currently there are two variants of {{rename()}} sitting in {{FSDirectory}}.
> Both implementation shares quite a bit of common code.
> This jira proposes to clean up these two variants and extract the common code.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
