You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Mike Yoder <my...@cloudera.com> on 2014/11/11 01:22:19 UTC
Review Request 27846: SENTRY-140 - Garbage collect orphaned privileges
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/
-----------------------------------------------------------
Review request for sentry.
Bugs: SENTRY-140
https://issues.apache.org/jira/browse/SENTRY-140
Repository: sentry
Description
-------
Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f6699d2
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 2e829d6
Diff: https://reviews.apache.org/r/27846/diff/
Testing
-------
Wrote a test case; local testing.
Thanks,
Mike Yoder
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/#review60726
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102110>
Should exitRequired be an AtomicBoolean so that you don't we don't hang waiting for the lock if exit() is called? It adds another lock so maybe no...
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102111>
Do you need this? I don't see how run could ever be executed concurrently.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102103>
You need to worry about spurious wakups from the condition variable. I think this should be something like:
while (currentNotifies < NOTIFY_THRESHOLD && !exit) {
try {
a.wait();
} catch (InterruptedException) {}
}
notificationCount = 0;
//unlock
doDeletion();
Then notify every time there is a deletion.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102112>
If this throws an exception the thread will exit. Might be better to log an error instead.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102108>
I think the notification is an implementation detail. Really, this is incDeletionCount() and internally we do a notify.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102109>
form
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102107>
Comment on whether it is safe to call exit() multiple times.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102105>
Always put lock/unlock in try/finally blocks
- Lenni Kuff
On Nov. 11, 2014, 12:22 a.m., Mike Yoder wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27846/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2014, 12:22 a.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-140
> https://issues.apache.org/jira/browse/SENTRY-140
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
>
> This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f6699d2
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 2e829d6
>
> Diff: https://reviews.apache.org/r/27846/diff/
>
>
> Testing
> -------
>
> Wrote a test case; local testing.
>
>
> Thanks,
>
> Mike Yoder
>
>
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Mike Yoder <my...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/#review61287
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102829>
But I don't see any code paths where we'd hang waiting for the lock... and anyway, we deal with locking explicitly so I think we're alright. Another lock would confuse me.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102830>
I think you're right.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102838>
Done. Solved it a little differently...
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102844>
OK
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102845>
OK
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102846>
OK
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102847>
Done.
- Mike Yoder
On Nov. 11, 2014, 12:22 a.m., Mike Yoder wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27846/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2014, 12:22 a.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-140
> https://issues.apache.org/jira/browse/SENTRY-140
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
>
> This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f6699d2
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 2e829d6
>
> Diff: https://reviews.apache.org/r/27846/diff/
>
>
> Testing
> -------
>
> Wrote a test case; local testing.
>
>
> Thanks,
>
> Mike Yoder
>
>
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Mike Yoder <my...@cloudera.com>.
> On Nov. 13, 2014, 10:55 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, lines 938-957
> > <https://reviews.apache.org/r/27846/diff/2/?file=762600#file762600line938>
> >
> > Use getCount(MSentryPrivilege.class) instead?
Oh hey, that's handy.
> On Nov. 13, 2014, 10:55 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1573
> > <https://reviews.apache.org/r/27846/diff/2/?file=762600#file762600line1573>
> >
> > Make this configurable? Just thinking, does it make sense to have this parameter be in time units? So that it is deterministic how often this is called?
I first thought about time units - but the thing with that is we could be running that query over and over again without anything for it to do. We know when privs are potentially being orphaned, and when we think there might be some number of them we can clean them up. The only goal is to prevent "too many" of them lying around in the db. If, say, 9 of them hang out for a long time, that's really no harm done.
As for making it configurable... I think that's probably (hopefully?) not a useful knob for a user to turn; how exactly would we advise them to tune it, and under what circumstances? We'd also need to test a variety of different settings of this config value.
Prior to Cloudera, I focused on *removing* unnecessary tunables from customer-facing interfaces. :-) Hadoop is 180 degrees opposite of that thinking, I'll grant you.
Nonetheless, you've got more experience than I in this area and if you want me to, I'll put it in. It might in theory be useful to have around if there's a bug in the thread and the config could also shut off the thread. Let me know.
> On Nov. 13, 2014, 10:55 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java, lines 264-267
> > <https://reviews.apache.org/r/27846/diff/2/?file=762601#file762601line264>
> >
> > setTableName repeated twice?
Oops, fixed.
> On Nov. 13, 2014, 10:55 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 106
> > <https://reviews.apache.org/r/27846/diff/2/?file=762600#file762600line106>
> >
> > SentryStore namespace for an inner class?
Fixed.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/#review61323
-----------------------------------------------------------
On Nov. 13, 2014, 10:24 p.m., Mike Yoder wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27846/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2014, 10:24 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-140
> https://issues.apache.org/jira/browse/SENTRY-140
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
>
> This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f6699d2
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 2e829d6
>
> Diff: https://reviews.apache.org/r/27846/diff/
>
>
> Testing
> -------
>
> Wrote a test case; local testing.
>
>
> Thanks,
>
> Mike Yoder
>
>
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Mike Yoder <my...@cloudera.com>.
> On Nov. 13, 2014, 10:55 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1573
> > <https://reviews.apache.org/r/27846/diff/2/?file=762600#file762600line1573>
> >
> > Make this configurable? Just thinking, does it make sense to have this parameter be in time units? So that it is deterministic how often this is called?
>
> Mike Yoder wrote:
> I first thought about time units - but the thing with that is we could be running that query over and over again without anything for it to do. We know when privs are potentially being orphaned, and when we think there might be some number of them we can clean them up. The only goal is to prevent "too many" of them lying around in the db. If, say, 9 of them hang out for a long time, that's really no harm done.
>
> As for making it configurable... I think that's probably (hopefully?) not a useful knob for a user to turn; how exactly would we advise them to tune it, and under what circumstances? We'd also need to test a variety of different settings of this config value.
>
> Prior to Cloudera, I focused on *removing* unnecessary tunables from customer-facing interfaces. :-) Hadoop is 180 degrees opposite of that thinking, I'll grant you.
>
> Nonetheless, you've got more experience than I in this area and if you want me to, I'll put it in. It might in theory be useful to have around if there's a bug in the thread and the config could also shut off the thread. Let me know.
>
> Sravya Tirukkovalur wrote:
> Yeah, I see that. Many times making some things configurable can do more bad than good :-). I am only worried that value 10 might be too low, that this is fired too often and could cause performance regression. As far as I understand, having orphaned privileges is bad for following reasons: 1. Slight performance hit in sql queries going on SENTRY_DB_PRIVILEGE, this might not be too bad unless our ratio of orphans is pretty high? 2. Increased unnecessary memory foot print for services which cache this data (Impala and Namenode). I am not entirely sure if they handle orphans actually. So if you think of it, it is not too bad. But if the user is granting/revoking bulk privileges, this clean up might be called multiple times successively which can clog the sentry service? May be threshold around 50/100 would be safe?
Seems reasonable. I'll change it to 50.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/#review61323
-----------------------------------------------------------
On Nov. 14, 2014, 7:11 p.m., Mike Yoder wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27846/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2014, 7:11 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-140
> https://issues.apache.org/jira/browse/SENTRY-140
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
>
> This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f6699d2
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 2e829d6
>
> Diff: https://reviews.apache.org/r/27846/diff/
>
>
> Testing
> -------
>
> Wrote a test case; local testing.
>
>
> Thanks,
>
> Mike Yoder
>
>
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
> On Nov. 13, 2014, 10:55 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1573
> > <https://reviews.apache.org/r/27846/diff/2/?file=762600#file762600line1573>
> >
> > Make this configurable? Just thinking, does it make sense to have this parameter be in time units? So that it is deterministic how often this is called?
>
> Mike Yoder wrote:
> I first thought about time units - but the thing with that is we could be running that query over and over again without anything for it to do. We know when privs are potentially being orphaned, and when we think there might be some number of them we can clean them up. The only goal is to prevent "too many" of them lying around in the db. If, say, 9 of them hang out for a long time, that's really no harm done.
>
> As for making it configurable... I think that's probably (hopefully?) not a useful knob for a user to turn; how exactly would we advise them to tune it, and under what circumstances? We'd also need to test a variety of different settings of this config value.
>
> Prior to Cloudera, I focused on *removing* unnecessary tunables from customer-facing interfaces. :-) Hadoop is 180 degrees opposite of that thinking, I'll grant you.
>
> Nonetheless, you've got more experience than I in this area and if you want me to, I'll put it in. It might in theory be useful to have around if there's a bug in the thread and the config could also shut off the thread. Let me know.
Yeah, I see that. Many times making some things configurable can do more bad than good :-). I am only worried that value 10 might be too low, that this is fired too often and could cause performance regression. As far as I understand, having orphaned privileges is bad for following reasons: 1. Slight performance hit in sql queries going on SENTRY_DB_PRIVILEGE, this might not be too bad unless our ratio of orphans is pretty high? 2. Increased unnecessary memory foot print for services which cache this data (Impala and Namenode). I am not entirely sure if they handle orphans actually. So if you think of it, it is not too bad. But if the user is granting/revoking bulk privileges, this clean up might be called multiple times successively which can clog the sentry service? May be threshold around 50/100 would be safe?
- Sravya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/#review61323
-----------------------------------------------------------
On Nov. 14, 2014, 7:11 p.m., Mike Yoder wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27846/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2014, 7:11 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-140
> https://issues.apache.org/jira/browse/SENTRY-140
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
>
> This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f6699d2
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 2e829d6
>
> Diff: https://reviews.apache.org/r/27846/diff/
>
>
> Testing
> -------
>
> Wrote a test case; local testing.
>
>
> Thanks,
>
> Mike Yoder
>
>
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/#review61323
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102879>
SentryStore namespace for an inner class?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102880>
Use getCount(MSentryPrivilege.class) instead?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment102883>
Make this configurable? Just thinking, does it make sense to have this parameter be in time units? So that it is deterministic how often this is called?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
<https://reviews.apache.org/r/27846/#comment102882>
setTableName repeated twice?
- Sravya Tirukkovalur
On Nov. 13, 2014, 10:24 p.m., Mike Yoder wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27846/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2014, 10:24 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-140
> https://issues.apache.org/jira/browse/SENTRY-140
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
>
> This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f6699d2
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 2e829d6
>
> Diff: https://reviews.apache.org/r/27846/diff/
>
>
> Testing
> -------
>
> Wrote a test case; local testing.
>
>
> Thanks,
>
> Mike Yoder
>
>
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/#review62653
-----------------------------------------------------------
Ship it!
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment104720>
Do you want to always rollback this tx?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment104722>
Verify that idList really is empty when rollback is true by adding
Preconditions.checkState(!rollback);
after this if block.
- Lenni Kuff
On Nov. 21, 2014, 7:47 p.m., Mike Yoder wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27846/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 7:47 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-140
> https://issues.apache.org/jira/browse/SENTRY-140
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
>
> This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3615661
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 03ed378
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 70917b7
>
> Diff: https://reviews.apache.org/r/27846/diff/
>
>
> Testing
> -------
>
> Wrote a test case; local testing.
>
>
> Thanks,
>
> Mike Yoder
>
>
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Mike Yoder <my...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/#review62796
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment104917>
It looks odd, but I do, actually. I got this paradigm directly from here: http://www.datanucleus.org/products/datanucleus/jdo/transactions.html#readonly
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment104927>
Added. But intellij actually complained that rollback would always be false at that point. I think that intellij is right - by the time idList is added to, there aren't any more places where an exception could occur. But anyway, better safe than sorry.
- Mike Yoder
On Nov. 21, 2014, 7:47 p.m., Mike Yoder wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27846/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 7:47 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-140
> https://issues.apache.org/jira/browse/SENTRY-140
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
>
> This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3615661
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 03ed378
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 70917b7
>
> Diff: https://reviews.apache.org/r/27846/diff/
>
>
> Testing
> -------
>
> Wrote a test case; local testing.
>
>
> Thanks,
>
> Mike Yoder
>
>
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Mike Yoder <my...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/
-----------------------------------------------------------
(Updated Nov. 24, 2014, 8:51 p.m.)
Review request for sentry.
Bugs: SENTRY-140
https://issues.apache.org/jira/browse/SENTRY-140
Repository: sentry
Description
-------
Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3615661
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 03ed378
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 70917b7
Diff: https://reviews.apache.org/r/27846/diff/
Testing
-------
Wrote a test case; local testing.
Thanks,
Mike Yoder
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Mike Yoder <my...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/
-----------------------------------------------------------
(Updated Nov. 21, 2014, 7:47 p.m.)
Review request for sentry.
Changes
-------
Finally, have a working version. Fixed some bugs in the cleanup thread loop, and most importantly rewrote the way the cleanup thread finds objects into a two-step process: first find candidate privilege objects that might be unassociated with a role, and then get those objects via DN, verify they have no roles, and delete them.
Bugs: SENTRY-140
https://issues.apache.org/jira/browse/SENTRY-140
Repository: sentry
Description
-------
Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3615661
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 03ed378
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 70917b7
Diff: https://reviews.apache.org/r/27846/diff/
Testing
-------
Wrote a test case; local testing.
Thanks,
Mike Yoder
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Mike Yoder <my...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/
-----------------------------------------------------------
(Updated Nov. 21, 2014, 12:57 a.m.)
Review request for sentry.
Bugs: SENTRY-140
https://issues.apache.org/jira/browse/SENTRY-140
Repository: sentry
Description
-------
Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3615661
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 03ed378
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 70917b7
Diff: https://reviews.apache.org/r/27846/diff/
Testing
-------
Wrote a test case; local testing.
Thanks,
Mike Yoder
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Mike Yoder <my...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/#review61747
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment103624>
Funny - Sravya and I were discussing a tunable here, too, either to determine the threshold and/or to enable/disable. Sravya was for, I against. But now it's 2-1 :-). I do agree that a way to disable is potentially a good idea, so I'll put it in.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment103742>
Resolved in person. Thanks!
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment103741>
Resolved in person. Thanks!
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment103648>
Resolved in person. Thanks!
- Mike Yoder
On Nov. 14, 2014, 7:44 p.m., Mike Yoder wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27846/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2014, 7:44 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-140
> https://issues.apache.org/jira/browse/SENTRY-140
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
>
> This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f6699d2
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 2e829d6
>
> Diff: https://reviews.apache.org/r/27846/diff/
>
>
> Testing
> -------
>
> Wrote a test case; local testing.
>
>
> Thanks,
>
> Mike Yoder
>
>
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/#review61328
-----------------------------------------------------------
Ship it!
Ship It!
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment103370>
It would probably be good to add a configuration option that allows disabling this functionality in case we find it causes problems in some scenarios.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment103372>
This lock also needs to be in a try/finally
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment103371>
nit: I don't believe that you actually will get this exception in the case of a spurious wakeup. I believe this exception is only thrown when another thread "interrupts" this thread. That's why usually this is implemented using a pattern like:
lock.lock();
try {
while(condition) {
cond.await()
}
// Do some stuff now that condition is met and
// you hold the lock.
} finally {
lock.unlock();
}
You might want two loops a while(true) to keep the thread running and then this loop.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/27846/#comment103373>
Do you need to (re)lock here? Instead could you just be optimistic and set this to 0 before executing removeOrphanPrivs.
- Lenni Kuff
On Nov. 14, 2014, 7:44 p.m., Mike Yoder wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27846/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2014, 7:44 p.m.)
>
>
> Review request for sentry.
>
>
> Bugs: SENTRY-140
> https://issues.apache.org/jira/browse/SENTRY-140
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
>
> This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f6699d2
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 2e829d6
>
> Diff: https://reviews.apache.org/r/27846/diff/
>
>
> Testing
> -------
>
> Wrote a test case; local testing.
>
>
> Thanks,
>
> Mike Yoder
>
>
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Mike Yoder <my...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/
-----------------------------------------------------------
(Updated Nov. 14, 2014, 7:44 p.m.)
Review request for sentry.
Changes
-------
Changing threshold to 50.
Bugs: SENTRY-140
https://issues.apache.org/jira/browse/SENTRY-140
Repository: sentry
Description
-------
Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f6699d2
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 2e829d6
Diff: https://reviews.apache.org/r/27846/diff/
Testing
-------
Wrote a test case; local testing.
Thanks,
Mike Yoder
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Mike Yoder <my...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/
-----------------------------------------------------------
(Updated Nov. 14, 2014, 7:11 p.m.)
Review request for sentry.
Changes
-------
Fixes after Sravya's review.
Bugs: SENTRY-140
https://issues.apache.org/jira/browse/SENTRY-140
Repository: sentry
Description
-------
Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f6699d2
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 2e829d6
Diff: https://reviews.apache.org/r/27846/diff/
Testing
-------
Wrote a test case; local testing.
Thanks,
Mike Yoder
Re: Review Request 27846: SENTRY-140 - Garbage collect orphaned
privileges
Posted by Mike Yoder <my...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27846/
-----------------------------------------------------------
(Updated Nov. 13, 2014, 10:24 p.m.)
Review request for sentry.
Changes
-------
Code changes based on review feedback.
Bugs: SENTRY-140
https://issues.apache.org/jira/browse/SENTRY-140
Repository: sentry
Description
-------
Due to the M-to-N relationship of roles to privileges, when a role is removed from a privilege (or vice versa) that privilege is not deleted from the database.
This fix introduces a thread that periodically (currently, once every 10 times a role and a privilege are disassociated) runs a SQL query that searches for all orphaned privileges and deletes them.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f6699d2
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 2e829d6
Diff: https://reviews.apache.org/r/27846/diff/
Testing
-------
Wrote a test case; local testing.
Thanks,
Mike Yoder