[
https://issues.apache.org/jira/browse/HDFS-4148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13496698#comment-13496698
]
Aaron T. Myers commented on HDFS-4148:
--------------------------------------
Hi Brandon,
One high-level comment:
In general I'm a little leery of adding these methods which are getters but
also have the side effect of checking for the presence of RO snapshots and
throwing an exception. Perhaps better would be to leave the getter methods
alone and instead add a function along the lines
"ensureNotInROSnapshot(INodesInPath)" which would either be a no-op or throw a
SnapshotAccessControlException. I think that'd make the code clearer. What do
you think?
One specific example of where this is unclear is that the method name
"isDirMutable" seems like it's returning true if the dir is mutable, when in
fact it's semantics are "isDirAndCheckMutable". If we choose to stick with the
getter-with-check option, I think we should change the names to
"getXAndEsnureMutable", instead of just "getXMutable".
A few little specific comments:
# The class comment in TestDisallowModifyROSnapshot is copied from TestSnapshot
and isn't accurate.
# Why was it necessary to use raw DFSClients in testCreate and
testCreateSymlink? Couldn't we have just used FileSystem (or FileContext) as
was done in the rest of the test cases? If indeed this isn't possible, it would
be good to add a comment explaining why.
# I don't understand this comment:
{code}
+ // TODO: if link is objInSnapshot, ParentNotDirectoryException got thrown
+ // first by verifyParentDir()
{code}
What's left to be done here?
# The method comment for ClientProtocol#createSymlink says "@throws
SnapshotAccessControlException if path is in RO snapshot." I think we should
make it clear that this is only if the _link path_ is in the RO snapshot. No
exception should be thrown if the _target path_ is in an RO snapshot.
# This patch copy/pastes the message "Modification on RO snapshot is
disallowed" in three places. I recommend you add a no-args constructor to
SnapshotAccessControlException with this message as the default, much as the
existing AccessControlException has done. I also recommend changing the message
to "Permission denied: cannot modify read-only snapshot."
# I'd recommend adding a method comment to FSDirectory#existsMutable, much as
you've done for FSDirectory#getMutableINode.
# There's some inconsistency in naming some of the getter methods along the
lines of "getXMutable" versus "getMutableX". Per my high-level comment above I
recommend we change all of these to "getXAndEnsureMutable".
Sorry that I'm providing this review so late. Given that this patch was just
committed, we can either revert the commit and update the patch, or address
this feedback in a follow-up JIRA. Either way is fine by me.
> Disallow write/modify operations on files and directories in a snapshot
> ------------------------------------------------------------------------
>
> Key: HDFS-4148
> URL: https://issues.apache.org/jira/browse/HDFS-4148
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: name-node
> Affects Versions: Snapshot (HDFS-2802)
> Reporter: Brandon Li
> Assignee: Brandon Li
> Fix For: Snapshot (HDFS-2802)
>
> Attachments: HDFS-4148.patch, HDFS-4148.patch, HDFS-4148.patch,
> HDFS-4148.patch, HDFS-4148.patch
>
>
> disallow modification on RO snapshots, including create, append,
> setReplication/Permission/Owner, rename, delete, makedir, setQuota/Time,
> createSymlink.
--
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
