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/02/16 17:04:54 UTC
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54947/
-----------------------------------------------------------
(Updated Feb. 16, 2017, 5:04 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Added changes to address review comments and fixed some other issues identified
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 (updated)
-----
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
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> 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
>
>
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54947/#review167142
-----------------------------------------------------------
- kalyan kumar kalvagadda
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/
>
>
> 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
>
>
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On March 9, 2017, 11:42 p.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 594 (patched)
> > <https://reviews.apache.org/r/54947/diff/6/?file=1655896#file1655896line618>
> >
> > Same comments as for line 572
persistedPriv could be null so we need to have the null check here.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54947/#review168543
-----------------------------------------------------------
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
>
>
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54947/#review168543
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 572 (patched)
<https://reviews.apache.org/r/54947/#comment240789>
1) space after if
2) Seems like persistedPriv can't be null here
3) Suppose that persistedPriv was created above at line 567. Since its getRoles().isEmpty() is true it wil never persist, while in the old code it would. What is the expected behavior here in such cases?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 594 (patched)
<https://reviews.apache.org/r/54947/#comment240792>
Same comments as for line 572
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 612 (original), 596 (patched)
<https://reviews.apache.org/r/54947/#comment240793>
space after if and another before {
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 619 (original), 607 (patched)
<https://reviews.apache.org/r/54947/#comment240794>
space after if and before {
- Alexander Kolbasov
On March 4, 2017, 12:40 a.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54947/
> -----------------------------------------------------------
>
> (Updated March 4, 2017, 12:40 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 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/6/
>
>
> 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
>
>
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54947/#review168820
-----------------------------------------------------------
- kalyan kumar kalvagadda
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
>
>
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54947/
-----------------------------------------------------------
(Updated March 24, 2017, 12:38 a.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Addressed review comments.
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 (updated)
-----
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 fc84e23
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 32637d2
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/8/
Changes: https://reviews.apache.org/r/54947/diff/7-8/
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
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
Addressed review comments.
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/54947/diff/6-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
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54947/
-----------------------------------------------------------
(Updated March 4, 2017, 12:40 a.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Addressed review comments.
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 (updated)
-----
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/6/
Changes: https://reviews.apache.org/r/54947/diff/5-6/
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
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
Addressed sasha's review comments.
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 (updated)
-----
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/
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
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
Addressed review comments.
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 (updated)
-----
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
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54947/
-----------------------------------------------------------
(Updated Feb. 22, 2017, 5:22 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 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
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54947/
-----------------------------------------------------------
(Updated Feb. 22, 2017, 5:18 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Bugs: SENTRY-1548
https://issues.apache.org/jira/browse/SENTRY-1548
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
Re: Review Request 54947: SENTRY-1556 Simplify privilege cleaning
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54947/#review165734
-----------------------------------------------------------
- kalyan kumar kalvagadda
On Feb. 16, 2017, 5:04 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54947/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2017, 5:04 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
>
>