[
https://issues.apache.org/jira/browse/HDFS-5600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13854394#comment-13854394
]
Chris Nauroth commented on HDFS-5600:
-------------------------------------
Hi, [~vinayrpet]. Thank you again for volunteering to do this. I'll keep you
informed as the rest of the implementation comes together and you'll be able to
test the CLI end-to-end. Meanwhile, here are a couple of comments about the
current patch that we'll want to address:
# {{FsCommand}}: minor: It looks like there is precedent for listing the
command classes in alphabetical order in {{registerCommands}}, so let's put
{{AclCommands}} at the top. (This is just a style nit. It doesn't change
functionality, because the help output always sorts the command names.)
# {{FsAction#getFSAction}}: This class has a cached copy of all values in the
{{vals}} static member, which you can reuse here instead of making a separate
call to {{FsAction#values}}. In the Javadoc, I wasn't sure what "3 String
representation" meant. Was that supposed to be "3-character String
representation"? Let's also have the Javadoc mention that it returns null if
not found.
# Thanks for getting started on tests to cover the validation logic.
Eventually, I think we'll want something like the other HDFS CLI XML-based
tests, i.e. {{org.apache.hadoop.cli.TestAclCLI}} and a corresponding
{{testAclCLIConf.xml}}.
# {{AclCommands}}: The short {{USAGE}} string literals should not have newline
characters at the end. Right now, when I run "hdfs dfs", I see the getfacl and
setfacl command usage slightly misformatted across multiple lines because of
this. Also, setfacl has an internal new line separating the 2 variants which
further misformats the output. We might need to separate the 2 variants with a
comma or find some other clever way to pack the usage onto a single line.
# {{AclCommands}}: Instead of calling {{System.out.println}}, please call
{{out.println}}. {{out}} is a {{PrintStream}} inherited from the {{Command}}
base class.
# {{AclCommands#processPath}}: getfacl also prints a file's special flags. In
HDFS, the only special flag we implement is sticky bit, so let's print that
information. We already made sticky bit available on the {{AclStatus}} object.
Here is example output from getfacl on a sticky directory on Linux. The
"flags" line should only be printed if the file has sticky bit:
{code}
mkdir sticky
chmod 01777 sticky
getfacl sticky
# file: sticky/
# owner: cnauroth
# group: cnauroth
# flags: --t
user::rwx
group::rwx
other::rwx
{code}
# For setfacl, we won't have support for the -d flag (at least not yet). There
won't be support for it at the API level, so please remove that option from the
CLI.
# {{AclCommands#SetfaclCommand#processOptions}}: I found it challenging to read
the conditional that checks for conflicting options (though I think your logic
is correct). Can you please try reformatting or perhaps introducing some
helper methods to improve readability?
# {{AclCommands#SetfaclCommand#parseAclSpec}}: The CLI needs to be able to
accept a mix of access ACL entries and default ACL entries in a single
acl_spec. Default ACL entries begin with "default:". Access ACL entries omit
this. For example:
{code}
setfacl -m user:vinay:rw-,default:user:cnauroth:r-- /file1
{code}
This means that the parsing logic needs to handle a {{split}} of length 3 or 4
(not just 3). If the length is 4, then the first element must be "default",
and the parser handles this by setting default scope on the entry builder.
# {{AclCommands#SetfaclCommand#parseAclSpec}}: Optional: You might be able to
simplify parsing of type by calling {{Enum.valueOf(AclEntryType.class, type)}}.
It might make error handling a little different though, so let me know what
you think.
> FsShell CLI: add getfacl and setfacl with minimal support for getting and
> setting ACLs.
> ---------------------------------------------------------------------------------------
>
> Key: HDFS-5600
> URL: https://issues.apache.org/jira/browse/HDFS-5600
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: tools
> Affects Versions: HDFS ACLs (HDFS-4685)
> Reporter: Chris Nauroth
> Assignee: Vinay
> Attachments: HDFS-5600.patch
>
>
> Implement and test FsShell CLI commands for getfacl and setfacl.
--
This message was sent by Atlassian JIRA
(v6.1.4#6159)
