[
https://issues.apache.org/jira/browse/HDFS-5608?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13880565#comment-13880565
]
Chris Nauroth commented on HDFS-5608:
-------------------------------------
Comments on the latest version of the patch:
# {{JsonUtil}}: It looks like we never would call the new {{toJsonString}} or
{{toAclStatus}} methods with {{includesType}} set to false. Can we remove the
{{includesType}} parameter and the code paths that would have handled the
{{false}} case? That way, we won't have dormant, untested code.
# {{JsonUtil#toAclStatus}}: Inside the for loop, we're still repeating parsing
logic that also exists in {{AclEntry#parseAclSpec}}. I think we need one more
refactoring in the common code to give us a static {{AclEntry#parseAclEntry}}
method that parses a single ACL entry (not a list like
{{AclEntry#parseAclSpec}}). I'll let you know when that's available.
# {{AclPermissionParam}}: The constructor makes 3 separate calls to
{{parseAclSpec}}. That method cannot return null, so you can cut down to 2
calls by removing the null check.
# {{AclPermissionParam#parseAclSpec}}: There is a typo in the variable name
{{aclspce}}. Inside the for loop, you can use {{AclEntry#toString}} instead of
building the string yourself. This code has some bugs like not prepending
"default" for a default ACL and a {{NullPointerException}} getting the
permission symbol if no permission is defined (as {{removeAclEntries}} does).
You'll get them all fixed for free if you switch to {{AclEntry#toString}}.
Actually, it seems like this whole method is equivalent to this one-liner:
{{StringUtils.join(",", entries)}}. Try that out.
# {{TestJsonUtil}}: Thanks for adding tests! I recently added a class named
{{AclTestHelpers}} that contains some static methods that can shorten test code
that needs to make ACL entries. You can see an example of how this is used in
places like {{TestNameNodeAcl}}.
# In addition to the tests already in the patch, we'll also need to add
functional tests that exercise each of the new APIs end-to-end using a
{{MiniDFSCluster}}. Let's add these tests in
{{org.apache.hadoop.hdfs.web.TestWebHDFS}}.
I think we'll be ready to commit after the above is addressed, so we're getting
close. :-) Thank you for incorporating the feedback.
> 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, HDFS-5608.2.patch,
> HDFS-5608.3.patch
>
>
> Implement and test {{GETACLS}} and {{SETACL}} in WebHDFS.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
