hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Nauroth (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-5608) WebHDFS: implement GETACLSTATUS and SETACL.
Date Fri, 24 Jan 2014 00:50:38 GMT

    [ 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.

> -------------------------------------------
>                 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

View raw message