> On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > > Lines 139 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650854#file1650854line139> > > > > 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.
Changed done to these function are reverted. They are no more needed. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > > Lines 141 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650854#file1650854line141> > > > > The removal isn't done here, so the comment should be moved elsewhere. Changed done to these function are reverted. They are no more needed. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > > Lines 145 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650854#file1650854line145> > > > > pm isn't used here Changed done to these function are reverted. They are no more needed. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > > Line 138 (original), 146 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650854#file1650854line146> > > > > Please add comment explaining why the copy is needed here Changed done to these function are reverted. They are no more needed. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > > Line 142 (original), 150 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650854#file1650854line150> > > > > How is the removal from pm going to happen? Changed done to these function are reverted. They are no more needed. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > > Line 176 (original), 184 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650854#file1650854line184> > > > > Same comment as for removeGMPrivilege Changed done to these function are reverted. They are no more needed. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > > Lines 191 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650854#file1650854line191> > > > > Please explain why the copy is needed. Changed done to these function are reverted. They are no more needed. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 597 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650855#file1650855line618> > > > > Hmm - we get persistedPriv from the caller - are you sure we want to > > overwrite it? Can you clarify? If you look at the caller, persistedPriv is not always a persitant privilege. if (persistedPriv == null) { persistedPriv = convertToMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege)); } Here you can see that persistedPriv is constructed from currentPrivilege. Which is a constructed privilege. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 798 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650855#file1650855line819> > > > > 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? I have changed the name of this function. I feel it's better to have this a another function for re-usability. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 828 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650855#file1650855line852> > > > > 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. Functions removePrivileges and removeGMPrivileges which are called in this function actually update the privilege list in the sentryrole. If the list is not copied we will not have a complete list for audit. Agree, we do not need a hashset will change it to a Arraylist. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 829 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650855#file1650855line853> > > > > <> will work They are not the same. When we start adding elements into the containers, there is a difference on the memory is allocated. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 997 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650855#file1650855line1021> > > > > Is this used outside of tests? No, they are not. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Line 1897 (original), 1954 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650855#file1650855line1978> > > > > Is it still used now? removeOrphanedPrivileges is renamed to findOrphanedPrivileges. It is used in unit tests. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Line 2018 (original), 1966 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650855#file1650855line2099> > > > > Is this still needed? Yes, I added unit tests that use this function. > On Feb. 28, 2017, 7:35 a.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > > Lines 517 (patched) > > <https://reviews.apache.org/r/54947/diff/4/?file=1650857#file1650857line517> > > > > Formatting. > > > > The whle function makes no sense with the new changes. Agree. That is what i said in the comment. I just was waiting to hear from reviewers to remove it. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54947/#review167019 ----------------------------------------------------------- On Feb. 28, 2017, 9:30 p.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, 9:30 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/5/ > > > 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 > >
