----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54947/#review168543 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 572 (patched) <https://reviews.apache.org/r/54947/#comment240789> 1) space after if 2) Seems like persistedPriv can't be null here 3) Suppose that persistedPriv was created above at line 567. Since its getRoles().isEmpty() is true it wil never persist, while in the old code it would. What is the expected behavior here in such cases? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 594 (patched) <https://reviews.apache.org/r/54947/#comment240792> Same comments as for line 572 sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 612 (original), 596 (patched) <https://reviews.apache.org/r/54947/#comment240793> space after if and another before { sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 619 (original), 607 (patched) <https://reviews.apache.org/r/54947/#comment240794> space after if and before { - Alexander Kolbasov On March 4, 2017, 12:40 a.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54947/ > ----------------------------------------------------------- > > (Updated March 4, 2017, 12:40 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, > and Vadim Spector. > > > Bugs: SENTRY-1556 > https://issues.apache.org/jira/browse/SENTRY-1556 > > > Repository: sentry > > > Description > ------- > > SENTRY-1556 Simplify privilege cleaning > > I made changes for the first approach by removing privileges moment they are > not associated to any role. > I have identified below scenarios which this will happen > When a role is deleted, we can see if the associated privileges are > associated to any other roles. All the privileges that are not associated to > any roles can be deleted from storage > When a privilege is revoked for a role, we can remove the privilege from > storage if it is not associated to any role > > Note: Once this approached is reviewed and accepted, we need not call > PrivCleaner periodically for cleanup > > > Diffs > ----- > > > sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > f9fb0f3 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 27dbfca > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > f1d7a86 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 6d54748 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java > 29134fe > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 20ce392 > > > Diff: https://reviews.apache.org/r/54947/diff/6/ > > > Testing > ------- > > Added unit tests the scenarios mentioned in description. > Also tested the same with Sentry thrift client. > Run unit test cases of TestSentryStore. > > > Thanks, > > kalyan kumar kalvagadda > >
