----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54947/#review168860 -----------------------------------------------------------
Fix it, then Ship it! Ship It! sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java Line 176 (original), 176 (patched) <https://reviews.apache.org/r/54947/#comment241149> Thank you for commenting this method! sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java Line 177 (original), 177 (patched) <https://reviews.apache.org/r/54947/#comment241150> updates *and* privilege object ? You may reword the comment a bit, eg. : "We can't just iterate this.privileges since removeRole() call below will modify the privileges set while we walk it. So we take a snapshot and use it to visit all privileges." sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 568 (patched) <https://reviews.apache.org/r/54947/#comment241151> You can reword a comment a bit, for example: "The privilege corresponding to the currentPrivilege doesn't exist in the persistent store, so we create a fake one for the code below. The fake one is not associated with any role and shouldn't be stored in the persistent storage." sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 982 (patched) <https://reviews.apache.org/r/54947/#comment241152> null - Alexander Kolbasov On March 13, 2017, 8:20 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54947/ > ----------------------------------------------------------- > > (Updated March 13, 2017, 8:20 p.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/7/ > > > 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 > >
