[
https://issues.apache.org/jira/browse/HDFS-4446?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13574026#comment-13574026
]
Jing Zhao commented on HDFS-4446:
---------------------------------
The patch looks good and with the file diff we can simplify the logic a lot.
Some of my thoughts:
1. In AbstractINodeDiffList.java, the methods append/insert can be renamed to
addLast/addFirst just like Deque?
2. Seems saveChild2Snapshot and createSnapshotCopy are only useful for
INodeSymlink#recordModification now. I think we can make
INode#recordModification abstract, and move the
original INode#recordModification implementation to INodeSymlink.
3. INodeDirectorySnapshottable#replaceSelf is called when resetting a
snapshottable dir. When this dir has no snapshots but its directory diff list
contains its ancestors' directory diff, the current
INodeDirectoryWithSnapshot(INodeDirectory that) may lose all the previous diffs.
4. In INodeFileWithSnapshot#recordModification, is the check "if (this ==
getParent().getChild(getLocalNameBytes(), latest))" used to handle the corner
case that user may delete and then create with the same file name? Maybe we can
add a comment to explain.
5. FSDirectory#unprotectedReplaceINodeFile and FSDirectory#unprotectedDelete
may need more javadoc to explain the logic.
6. In INodeFileWithSnapshot#destroySubtreeAndCollectBlocks, if snapshot is not
null, is it possible that Util.collectBlocksAndClear may run twice? Also the
snapshot deletion has problems when handling several special cases: 1) we need
to go through the whole directory diff list when processing recursively to
handle the case that a file/directory containing the to-be-deleted snapshot
diff was deleted in a later snapshot. 2) when the diff list contains diff for
snapshot s0, s2, and s3, currently we simply combine s2 to s0 when deleting
snapshot s2. This will cause bug when a user later wants to visit snapshot s1.
We can create a separate jira to fix this.
Besides, the comment "// TODO: add a new testcase for this" can be removed from
AbstractINodeDiffList#deleteSnapshotDiff (I forgot to remove it in
HDFS-4414...).
> Support file snapshot with diff lists
> -------------------------------------
>
> Key: HDFS-4446
> URL: https://issues.apache.org/jira/browse/HDFS-4446
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: namenode
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Attachments: h4446_20130131.patch, h4446_20130203.patch,
> h4446_20130204b.patch, h4446_20130204.patch, h4446_20130205.patch
>
>
> Similar to INodeDirectoryWithSnapshot, it is better to implement
> INodeFileWithSnapshot so that the INodeFileSnapshot class can be eliminated.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
