hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aaron T. Myers (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-4148) Disallow write/modify operations on files and directories in a snapshot
Date Wed, 14 Nov 2012 00:10:14 GMT

    [ 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

Mime
View raw message