You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Sravya Tirukkovalur <sr...@cloudera.com> on 2015/06/02 01:00:34 UTC
Re: Review Request 31070: Sentry-656: Update SentryStore for
import/export feature
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31070/#review85656
-----------------------------------------------------------
Overall, I think there are many avoidable iterations happening over the maps, which also makes it hard to follow the code. I see that might be a by product of trying to reuse existing functions. But I think we should keep the logic simple. One way to look at it is: Set<MGroup> and Set<MRoles> is all that we need from SentryStore. And we can get the group->roles and role->privileges from these objects and build required Thrift response in the processor.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/31070/#comment137331>
Fyi: I believe we always internally store * rather than ALL, in which case we do not have to handle both of these here. But it is either ways good to be handling it. Thanks!
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/31070/#comment137332>
Why not handle transaction consistency in this function? I see that this function is being called from line 2276 and rollback logic incase of exceptions is not handled there.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/31070/#comment137382>
Might be good to name functions more specifically, so that it is straight forward to guess what this is expected to return, especially as there are multiple data structures for the same thing. Like, may be getTGroupRoleNamesMap? Similarly for other functions as well.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/31070/#comment137370>
Why cant we add this check on line 2020?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/31070/#comment137378>
This looks like will lead to a redundant call msentryrole -> rolename -> msentryrole.
May be you can create another function like getAllTSentryPrivilegesByRoleName(mSentryRole) instead?
- Sravya Tirukkovalur
On May 20, 2015, 1:29 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31070/
> -----------------------------------------------------------
>
> (Updated May 20, 2015, 1:29 a.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryStore for import/export feature
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java d7937d0
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31070/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 31070: Sentry-656: Update SentryStore for
import/export feature
Posted by Colin Ma <ju...@intel.com>.
> On June 1, 2015, 11 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, lines 831-835
> > <https://reviews.apache.org/r/31070/diff/4/?file=964673#file964673line831>
> >
> > Why not handle transaction consistency in this function? I see that this function is being called from line 2276 and rollback logic incase of exceptions is not handled there.
The drop function is called in line 2276(in last code version) as the following order:
importSentryMetaData -> dropDuplicatedRoleForImport -> dropSentryRoleCore
The rollback logic is in importSentryMetaData to ensure all db operations are in one transaction.
> On June 1, 2015, 11 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 2017
> > <https://reviews.apache.org/r/31070/diff/4/?file=964673#file964673line2017>
> >
> > Might be good to name functions more specifically, so that it is straight forward to guess what this is expected to return, especially as there are multiple data structures for the same thing. Like, may be getTGroupRoleNamesMap? Similarly for other functions as well.
Update some functions according to the comments.
> On June 1, 2015, 11 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 2056
> > <https://reviews.apache.org/r/31070/diff/4/?file=964673#file964673line2056>
> >
> > Why cant we add this check on line 2020?
Thanks for the comments, update the patch, and simplify the code.
> On June 1, 2015, 11 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 2078
> > <https://reviews.apache.org/r/31070/diff/4/?file=964673#file964673line2078>
> >
> > This looks like will lead to a redundant call msentryrole -> rolename -> msentryrole.
> >
> > May be you can create another function like getAllTSentryPrivilegesByRoleName(mSentryRole) instead?
Good catch!! Update the code and no more redundant call for this method, thanks for your all comments!!
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31070/#review85656
-----------------------------------------------------------
On June 2, 2015, 8:40 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31070/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 8:40 a.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryStore for import/export feature
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java d7937d0
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31070/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>