----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39928/#review105189 -----------------------------------------------------------
Thanks for your patch Hao! Mostly looks good to me. Left some minor comments. sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 207) <https://reviews.apache.org/r/39928/#comment163609> Can we actually log an error? If log level is anything more than debug, it silently fails and can be confusing for users. sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 415) <https://reviews.apache.org/r/39928/#comment163611> I believe that you are not checking the authzInfo.doesBelongToAuthzObject(pathElements) to keep it symmetric with addAclFeature. If that is the case, can you update the jira/RB description and file a follow on jira which corrects this behavior for both add/removeAclFeature? sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java (line 198) <https://reviews.apache.org/r/39928/#comment163612> You may want to assert that the acl has been actually added and removed? sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java (line 205) <https://reviews.apache.org/r/39928/#comment163613> Also add a acl add and remove test for pathInside? - Sravya Tirukkovalur On Nov. 4, 2015, 3:51 a.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39928/ > ----------------------------------------------------------- > > (Updated Nov. 4, 2015, 3:51 a.m.) > > > Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > Paths that are sentry managed should not succesfully chmod/chown/removeACL. > We should update setGroup/setUser/setPermission and removeAclFeature. > > Old behavior: > chmod/chown > if not under prefix + unmanaged: writes to hdfs. > if managed: writes to hdfs. > Removing acls: > if not under prefix + unmanaged: removes from hdfs. > if managed: removes from hdfs. > > New behavior: > If not under prefix + unmanaged: writes to/removes from hdfs. > If managed : no op. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java > 419ab68e0d03f995c55d229b762453468de47571 > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java > fd5146f079d93687738a522f42beaa59031a4f82 > > Diff: https://reviews.apache.org/r/39928/diff/ > > > Testing > ------- > > Added several new unit tests for setPermission/setUser/setGroup/removeAcl > cases validation. > > > Thanks, > > Hao Hao > >
