[
https://issues.apache.org/jira/browse/HDFS-5608?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13870172#comment-13870172
]
Chris Nauroth commented on HDFS-5608:
-------------------------------------
[[email protected]], thank you for addressing the feedback. Here are
some additional comments based on the new patch:
# {{DFSConfigKeys}}: The regex does not allow for default ACL entries.
Basically, these look identical to the access entries but have "default:"
prepended. Also, the regex only allows entries of type "user" or "group", so
it would reject the "mask" and "other" entries. Also, the regex does not allow
for an ACL entry that does not have a permission. For {{removeAclEntries}},
the user supplies an ACL spec with no permissions in the entries. You might
want to take a look at the CLI implementation and HADOOP-10213 for more
examples of this.
# {{JsonUtil#toJsonString}}: I'm curious if we can skip the manual conversion
to {{entriesStringlist}} and just pass the {{List<AclEntry>}} directly. Will
the JSON conversion automatically use the {{toString}} representation? If this
conversion really is necessary, then a small improvement would be to use
Guava's {{Lists.newArrayListWithCapacity}} and pass {{entries.size()}} for the
capacity to prevent allocating too large of an array or causing an immediate
reallocation if the default initial array size turns out to be too small.
# {{JsonUtil#toAclStatus}}: Some of my earlier comments about the regex are
applicable here too. This method needs to be able to handle default ACL
entries and call {{setScope}} on the builder. It needs to be able to handle
the mask entry. Use "other" instead of "others" (singular, not plural).
# {{AclPermissionParam#DEFAULT}}: Was the default value meant to be empty
string? I don't think there is any specific ACL value that we could choose as
the default, because it could risk accidentally expanding access if the caller
forgets to provide the query parameter.
# {{AclPermissionParam#parseAclSpec}}: This is another spot where the earlier
feedback on the regex has an impact. This logic is very similar to
{{AclCommands#SetfaclCommand#parseAclSpec}}. Can we work out a way for both
the CLI and WebHDFS to use a common method? The parsing logic would be the
same for both.
# {{PutOpParam}}: I think {{GETACLS}} needs to be removed.
# Nitpick: the Hadoop project code standard wraps lines at 80 characters,
indents code blocks by 2 spaces, and uses spaces (not tabs) for indentation.
There are a few places in the patch that need to be converted to this standard.
> WebHDFS: implement GETACLSTATUS and SETACL.
> -------------------------------------------
>
> Key: HDFS-5608
> URL: https://issues.apache.org/jira/browse/HDFS-5608
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: webhdfs
> Affects Versions: HDFS ACLs (HDFS-4685)
> Reporter: Chris Nauroth
> Assignee: Sachin Jose
> Attachments: HDFS-5608.0.patch, HDFS-5608.1.patch
>
>
> Implement and test {{GETACLS}} and {{SETACL}} in WebHDFS.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
