----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54947/#review167019 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java (line 139) <https://reviews.apache.org/r/54947/#comment239128> It is a bit difficult to parse. The method walks through all generic model privileges for this role and removes this role from their roles set. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java (line 141) <https://reviews.apache.org/r/54947/#comment239129> The removal isn't done here, so the comment should be moved elsewhere. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java (line 145) <https://reviews.apache.org/r/54947/#comment239127> pm isn't used here sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java (line 146) <https://reviews.apache.org/r/54947/#comment239131> Please add comment explaining why the copy is needed here sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java (line 150) <https://reviews.apache.org/r/54947/#comment239133> How is the removal from pm going to happen? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java (line 184) <https://reviews.apache.org/r/54947/#comment239130> Same comment as for removeGMPrivilege sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java (line 191) <https://reviews.apache.org/r/54947/#comment239134> Please explain why the copy is needed. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 224) <https://reviews.apache.org/r/54947/#comment239137> This probably should be close() rather then stop() but it is outside the scope of this change. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 597) <https://reviews.apache.org/r/54947/#comment239138> Hmm - we get persistedPriv from the caller - are you sure we want to overwrite it? Can you clarify? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 599) <https://reviews.apache.org/r/54947/#comment239139> Extra line sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 611) <https://reviews.apache.org/r/54947/#comment239140> extra line sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 647) <https://reviews.apache.org/r/54947/#comment239141> formatting - should be } else { sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 798) <https://reviews.apache.org/r/54947/#comment239142> There is already a method with this name doing a different thing. Can you rename this one? Also, this is only called by getAllMSentryPrivileges() - do you need a special function for this? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 799) <https://reviews.apache.org/r/54947/#comment239143> paramBuilder is never used. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 801) <https://reviews.apache.org/r/54947/#comment239144> You can just return (List<MSentryPrivilege>) query.execute(); sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 822) <https://reviews.apache.org/r/54947/#comment239145> result is never used sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 823) <https://reviews.apache.org/r/54947/#comment239146> space between comma and next parameter sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 827) <https://reviews.apache.org/r/54947/#comment239151> Please document this function sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 828) <https://reviews.apache.org/r/54947/#comment239147> Why do you need a copy? Do we need a hashset? A simple linked list or arraylist should be sufficient. Also, there is no need to specify <MSentryPrivilege> in HashSet declaration, <> will work. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 829) <https://reviews.apache.org/r/54947/#comment239148> <> will work sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 831) <https://reviews.apache.org/r/54947/#comment239149> pm isn't used there sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 836) <https://reviews.apache.org/r/54947/#comment239150> formatting. if (condition) { something } sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 985) <https://reviews.apache.org/r/54947/#comment239155> Please document all new methods sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 986) <https://reviews.apache.org/r/54947/#comment239153> You already have getMSentryPrivilege, please choose another name. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 988) <https://reviews.apache.org/r/54947/#comment239152> return tm.executeTransaction( new TransactionBlock<MSentryPrivilege>() { public MSentryPrivilege execute(... sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 990) <https://reviews.apache.org/r/54947/#comment239154> just return getMSentryPrivilege(tPrivilege, pm); sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 997) <https://reviews.apache.org/r/54947/#comment239156> Is this used outside of tests? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 998) <https://reviews.apache.org/r/54947/#comment239157> Same comment about providing type for Transaction block. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1713) <https://reviews.apache.org/r/54947/#comment239158> Comments style: /* * ... */ Better to use // comments sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1954) <https://reviews.apache.org/r/54947/#comment239159> Is it still used now? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1966) <https://reviews.apache.org/r/54947/#comment239160> Is this still needed? sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java (line 348) <https://reviews.apache.org/r/54947/#comment239161> Comment formatting sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java (line 517) <https://reviews.apache.org/r/54947/#comment239162> Formatting. The whle function makes no sense with the new changes. - Alexander Kolbasov On Feb. 28, 2017, 12:47 a.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54947/ > ----------------------------------------------------------- > > (Updated Feb. 28, 2017, 12:47 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 > 6dc6918 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 7b926a5 > > 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 > 17a4a93 > > 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. > Run unit test cases of TestSentryStore. > > > Thanks, > > kalyan kumar kalvagadda > >
