----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54947/#review162570 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java (line 138) <https://reviews.apache.org/r/54947/#comment233903> Please add javadoc for this method sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java (line 181) <https://reviews.apache.org/r/54947/#comment233902> Please add javadoc to this method sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java (line 182) <https://reviews.apache.org/r/54947/#comment233901> Can you fix the old comment here - it should be 'removePrivilege' sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 572) <https://reviews.apache.org/r/54947/#comment233908> Since you are here, can you break the long line? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 573) <https://reviews.apache.org/r/54947/#comment233907> It is better to use isEmpty() rather then compare collection size with zero. If you do this, the switch() statement becomes clunky and probably will be better represented as a simple if This is espceially true here since you get the size twice sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 574) <https://reviews.apache.org/r/54947/#comment233909> Can it ever happen? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 577) <https://reviews.apache.org/r/54947/#comment233910> I think you can remove the whole case statement and just have these three lines (replacing size with isEmpty()) sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 602) <https://reviews.apache.org/r/54947/#comment233906> It is better to use isEmpty() rather then compare collection size with zero. If you do this, the switch() statement becomes clunky and probably will be better represented as a simple if sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 603) <https://reviews.apache.org/r/54947/#comment233911> This should never be the case, so you can simply remove role and then delete priv if roles set is empty sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 618) <https://reviews.apache.org/r/54947/#comment233905> It is better to use isEmpty() rather then compare collection size with zero. If you do this, the switch() statement becomes clunky and probably will be better represented as a simple if sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 619) <https://reviews.apache.org/r/54947/#comment233912> See comment for case statement above sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 806) <https://reviews.apache.org/r/54947/#comment233914> Please add documentation for this function Also it would be better to avoid overloading. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 807) <https://reviews.apache.org/r/54947/#comment233913> Extra emoty line 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/#comment233904> We no longer need numPrivs 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/#comment233916> It shouldn't be public - probably package-private Also it would be better to avoid overloading. It is very confusing - the same function name is used within the transaction and outside the transaction 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/#comment233915> This should be updated with generic transaction block sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1007) <https://reviews.apache.org/r/54947/#comment233917> package-private? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1691) <https://reviews.apache.org/r/54947/#comment233918> Please do not leave or reference deleted code - just explain what is going on. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1693) <https://reviews.apache.org/r/54947/#comment233919> Creating - is the sentence unfinished here? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1930) <https://reviews.apache.org/r/54947/#comment233920> Do we still need this comment? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1940) <https://reviews.apache.org/r/54947/#comment233921> Do we still need this? - Alexander Kolbasov On Jan. 17, 2017, 7:54 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54947/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2017, 7:54 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-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > f9fb0f32de23cfdc1bda3636d20a0e24d66cc1c6 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 57b6effa1b1bbfa22fbc2114db1e502365343207 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > 6dc6918314a11e343be281e86ecfa16ec4cf895a > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > b7ae63430cddb81cb3235a7b04ecf95410d8604f > > 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 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 17a4a937221891a72ee44db92976cfa5cab40bc4 > > 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 > >
