> 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 > >
