-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67131/#review203138
-----------------------------------------------------------




sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 92-93 (patched)
<https://reviews.apache.org/r/67131/#comment285223>

    should these be private final?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 95-102 (patched)
<https://reviews.apache.org/r/67131/#comment285234>

    I usually try to avoid throwing exceptions in the constructor because makes 
things easier to read and code.
    
    So I was wondering if we could make this constructor private and use a 
static method to initialize the arguments intead, like:
    
        public static HdfsAclEntity asGroup(String groupname) {
          return new HdfsAclEntity(AclEntryType.GROUP, groupname);
        }
    
        public static HdfsAclEntity asUser(String username) {
          return new HdfsAclEntity(AclEntryType.USER, username);
        }
        
    As you see, it above methods will avoid throwing exceptions. 
    Do you think we should do the same here?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 141-143 (original), 214-216 (patched)
<https://reviews.apache.org/r/67131/#comment285236>

    Should we have an if() that prevents constructing permissions if a 
TPrivilegeEntityType.GROUP is sent (which is not supported)?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 143-144 (original), 216-217 (patched)
<https://reviews.apache.org/r/67131/#comment285235>

    Why are you constructing permissions twice?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 157-158 (original), 236-242 (patched)
<https://reviews.apache.org/r/67131/#comment285238>

    Doesn't this if() be on the getPerms() function like it was on the 
getGrouopPerms?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Line 176 (original), 268-273 (patched)
<https://reviews.apache.org/r/67131/#comment285240>

    we could use HdfsAclEntity.asGroup(group) here to avoid catching exceptions 
that should not happen.



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Line 181 (original), 283-286 (patched)
<https://reviews.apache.org/r/67131/#comment285241>

    we could use HdfsAclEntity.asUser(user) here to avoid catching exceptions 
that should not happen.


- Sergio Pena


On May 15, 2018, 4:03 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67131/
> -----------------------------------------------------------
> 
> (Updated May 15, 2018, 4:03 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2174
>     https://issues.apache.org/jira/browse/SENTRY-2174
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> SentryAuthorizationProvider should now additionally provide the ACL entries 
> with the permissions that users have along with the permissions for the 
> groups.
> 
> With the changes proposed in SENTRY-2173, PrivilegeInfo will not only have 
> role to permission mapping. it will also have user to privilege mapping 
> information.
> 
> SentryAuthorizationProvider should be using the new information added in 
> PrivilegeInfo to add ACL for users.
> 
> 
> Diffs
> -----
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
>  a88d8e25ad745effe354aa6267252998b189a252 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryPermissions.java
>  dbce40538cf4721b601cf63b1da713f3d57fc981 
> 
> 
> Diff: https://reviews.apache.org/r/67131/diff/1/
> 
> 
> Testing
> -------
> 
> Added new unit tests and also made sure that all the existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Reply via email to