[
https://issues.apache.org/jira/browse/HDFS-6422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14005426#comment-14005426
]
Andrew Wang commented on HDFS-6422:
-----------------------------------
Hey Charles, thanks for the patch. I had some review comments:
* Rather than adding a boolean return type to FSDirectory#removeXAttr, why not
just throw an exception? It'll bubble all the way up.
* FSNamesystem, for the comments, would "EXECUTE on the" and "WRITE on the"
read better?
FSN#getXAttrs
* We might be able to get rid of {{filteredXAttrs}} since it looks like it's
just {{xAttrs}} now. It's somewhat misnamed now too, since the provided list is
no longer filtered by the PermissionFilter, it's checked.
* We shouldn't shortcut out if {{filteredXAttrs.isEmpty()}}, this didn't really
make sense in the first place since an exception should be thrown if you don't
have access to the path even if all your xattrs are filtered out. This should
be moved down to the {{else}} block after the permission checks.
* Line over 80 chars.
* When would {{toGet.size()}} be zero? I ask because I'd rather just inline the
Exception message than making it a constant. I think Java does the right thing
automatically, so there's no efficiency gain by pulling it out.
Tests:
* s/doSetXattr/doSetXAttr/. This function also moved in the diff for some
reason.
* TestDFSShell, Not sure what's going on here, the diff is hard to read. Was
removing testSetXAttrPermissionAsDifferentOwner intentional?
* If my comment about the incorrect shortcut is correct, it'd be good to have
tests on a no-permission and/or non-existent path to make sure that these
always throw an exception.
* I believe that the permission type is called "search" not "scan". It's
typically called "execute" for files and "search" for directories (see {{man
chmod(1)}}).
> getfattr in CLI doesn't throw exception or return non-0 return code when
> xattr doesn't exist
> --------------------------------------------------------------------------------------------
>
> Key: HDFS-6422
> URL: https://issues.apache.org/jira/browse/HDFS-6422
> Project: Hadoop HDFS
> Issue Type: Bug
> Affects Versions: 3.0.0
> Reporter: Charles Lamb
> Assignee: Charles Lamb
> Attachments: HDFS-6422.1.patch, HDFS-6422.2.patch, HDFS-6422.3.patch
>
>
> If you do
> hdfs dfs -getfattr -n user.blah /foo
> and user.blah doesn't exist, the command prints
> # file: /foo
> and a 0 return code.
> It should print an exception and return a non-0 return code instead.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
