> On May 16, 2018, 4:46 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java > > Lines 99-101 (original), 99-101 (patched) > > <https://reviews.apache.org/r/67131/diff/1-2/?file=2023041#file2023041line99> > > > > This exception is not neeed anymore because we are preventing to happen > > from the asGroup() and asUser() methods. So the exception will never happen.
HdfsAclEntity is inner class. Inspite making the constructor private, Methods in SentryPermissions can still instantiate HdfsAclEntity. That is why i did not remove the exception. > On May 16, 2018, 4:46 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java > > Lines 103 (patched) > > <https://reviews.apache.org/r/67131/diff/1-2/?file=2023041#file2023041line103> > > > > Isn't HdfsAclEntity.asUser() clear enough to let the developers that > > you're creating a user entity? I'm just trying to keep a cleaner api for > > us, but it is not necessary to change it. i feel "constructAclEntityForUser" is self explanatory. > On May 16, 2018, 4:46 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java > > Lines 275-279 (patched) > > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line278> > > > > will aclEntry be null? The static methods will never return null so we > > should be safe here. i left them as they are safety checks. I will remove it. > On May 16, 2018, 4:46 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java > > Lines 288 (patched) > > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line291> > > > > Can we have a comment here why FsAction.NONE is none for users and no > > for groups? there is a bug here. I will fix in the new patch. > On May 16, 2018, 4:46 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java > > Lines 290 (patched) > > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line293> > > > > aclEntry will never be null now that we use the static constructor, so > > we should be safe here. i left them as they are safety checks. I will remove it. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67131/#review203240 ----------------------------------------------------------- On May 15, 2018, 11:45 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, 11:45 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/2/ > > > Testing > ------- > > Added new unit tests and also made sure that all the existing tests pass. > > > Thanks, > > kalyan kumar kalvagadda > >
