You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by kalyan kumar kalvagadda <kk...@cloudera.com> on 2017/03/02 14:49:41 UTC
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
> 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
>
>