----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54947/#review160626 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java (line 504) <https://reviews.apache.org/r/54947/#comment231766> Just wondering, it makes perfect sense to call pm.deletePersistent for the privileges that have empty roles but for the else clause, we seem to call pm.makePersistent. Is this really required? - The updated privilege is already saved right? If it is not, where are they saving the privilege data into the database before this change? - Vamsee Yarlagadda On Jan. 5, 2017, 4:20 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54947/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2017, 4:20 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, > and Vadim Spector. > > > 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-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 12245ec6d1b28863e4c1fd7b265e60204732f0e6 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java > 55b61ac07c6865402ffe4d0ff1690afb435deb95 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > 4c3af7992c90ba6ce33ff38ca6c5a3eb3492dd8b > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > 0484eaade5853d13bc6671c6e0648c14f76116c0 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 742798dca6b3ad8c9b9eee1eea5380001d1d0cd9 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryPrivilege.java > PRE-CREATION > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java > 29134fec58e479e79aefb6cf96c6261698d64c08 > > Diff: https://reviews.apache.org/r/54947/diff/ > > > Testing > ------- > > Added unit tests the scenarios mentioned in description. > Also tested the same with Sentry thrift client. > > > Thanks, > > kalyan kumar kalvagadda > >
