[ 
https://issues.apache.org/jira/browse/HDFS-7087?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14953815#comment-14953815
 ] 

Xiao Chen commented on HDFS-7087:
---------------------------------

Thanks a lot Andrew for the review and comments! I have attached patch 002 to 
address your comments.
{quote}
Would appreciate javadoc and comments explaining the new methods, and also 
rationale for the new fake FileStatuses.
{quote}
Added to {{FSDirectory#createReservedStatuses}}, please see if they make sense 
to you.
{quote}
Any reason you set the sticky bit?
{quote}
I initially thought we should set it to indicate the dir is not intended for 
unprivileged deletion. On a second thought, this is not needed and confusing, 
so I removed it. 
{quote}
The isExactReservedName checks, noticed there's no check for rename. This seems 
brittle in general since we have to make sure it's present in everything 
besides list. Is the intent of these checks to provide consistent behavior? Do 
we think this is necessary, or can we simply rely on the existing behavior?
{quote}
I agree that it's fragile because we have to protect everything besides list. 
But we can’t rely on the existing behavior, because currently it’s protected by 
{{getFileInfo}} while parsing from the shell - before this patch, getFileInfo 
returns null for /.reserved so that the command isn't get processed and hence 
never reach to FSxxxxOp (except mkdirs). Now that we have to enable 
{{getFileInfo}} in order for ls to work, I think we have to protect other 
commands by ourselves. Below is a sample stack trace of the getFileInfo call.
{noformat}
         at 
org.apache.hadoop.io.retry.RetryInvocationHandler.invoke(RetryInvocationHandler.java:80)
         at com.sun.proxy.$Proxy19.getFileInfo(Unknown Source:-1)
         at org.apache.hadoop.hdfs.DFSClient.getFileInfo(DFSClient.java:1667)
         at 
org.apache.hadoop.hdfs.DistributedFileSystem$23.doCall(DistributedFileSystem.java:1269)
         at 
org.apache.hadoop.hdfs.DistributedFileSystem$23.doCall(DistributedFileSystem.java:1265)
         at 
org.apache.hadoop.fs.FileSystemLinkResolver.resolve(FileSystemLinkResolver.java:81)
         at 
org.apache.hadoop.hdfs.DistributedFileSystem.getFileStatus(DistributedFileSystem.java:1265)
         at org.apache.hadoop.fs.Globber.getFileStatus(Globber.java:62)
         at org.apache.hadoop.fs.Globber.doGlob(Globber.java:270)
         at org.apache.hadoop.fs.Globber.glob(Globber.java:149)
         at org.apache.hadoop.fs.FileSystem.globStatus(FileSystem.java:1670)
         at org.apache.hadoop.fs.shell.PathData.expandAsGlob(PathData.java:326)
         at org.apache.hadoop.fs.shell.Command.expandArgument(Command.java:239)
         at org.apache.hadoop.fs.shell.Command.expandArguments(Command.java:222)
         at 
org.apache.hadoop.fs.shell.FsCommand.processRawArguments(FsCommand.java:103)
         at org.apache.hadoop.fs.shell.Command.run(Command.java:166)
         at org.apache.hadoop.fs.FsShell.run(FsShell.java:310)
         at org.apache.hadoop.hdfs.TestDFSShell.runCmd(TestDFSShell.java:971)
{noformat}
{quote}
This enables listing /.reserved/raw/.reserved/, which is a little weird.
{quote}
Agree and good catch! Fixed.
{quote}
There are some acrobatics to get the cTime. Is this something we could set 
during FSImage loading? Would let us avoid the null check.
{quote}
Good idea, updated.

> Ability to list /.reserved
> --------------------------
>
>                 Key: HDFS-7087
>                 URL: https://issues.apache.org/jira/browse/HDFS-7087
>             Project: Hadoop HDFS
>          Issue Type: Improvement
>    Affects Versions: 2.6.0
>            Reporter: Andrew Wang
>            Assignee: Xiao Chen
>         Attachments: HDFS-7087.001.patch, HDFS-7087.002.patch, 
> HDFS-7087.draft.patch
>
>
> We have two special paths within /.reserved now, /.reserved/.inodes and 
> /.reserved/raw. It seems like we should be able to list /.reserved to see 
> them.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to