You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Prasad Mujumdar <pr...@cloudera.com> on 2015/05/11 23:15:51 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/#review83272
-----------------------------------------------------------
At high level,
- please see if the existing methods can be reused to load or update the meadata. Having multiple routines to do similar tasks make the code hard to maintain.
- It would be a good idea to provide an option of merging and overwriting the existing data during import.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/31070/#comment134175>
I think we are duplicating the code here for getting privileges for role. eg. there's already getAllTSentryPrivilegesByRoleName().
Please check if it's possible to consolidate the code for some of these operations.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/31070/#comment134193>
It would be useful to provide an option of merging and overwritting the existing roles
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/31070/#comment134194>
Should it handle merging of privilege, eg if you already have 'all' or appending 'all' then the others can be discarded.
- Prasad Mujumdar
On March 9, 2015, 2:49 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31070/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 2:49 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 136dab6
> 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 May 11, 2015, 9:15 p.m., Prasad Mujumdar wrote:
> > At high level,
> > - please see if the existing methods can be reused to load or update the meadata. Having multiple routines to do similar tasks make the code hard to maintain.
> > - It would be a good idea to provide an option of merging and overwriting the existing data during import.
Thanks for the comments, for the point1, the methods for import [group, role] and [role, privilege] are reimplemented, I think the new code will be easy to maintain using the exist method in SentryStore.
for point2, the option is added for import.
> On May 11, 2015, 9:15 p.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 2013
> > <https://reviews.apache.org/r/31070/diff/3/?file=888609#file888609line2013>
> >
> > I think we are duplicating the code here for getting privileges for role. eg. there's already getAllTSentryPrivilegesByRoleName().
> > Please check if it's possible to consolidate the code for some of these operations.
The code is updated for this problem, thanks.
> On May 11, 2015, 9:15 p.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 2162
> > <https://reviews.apache.org/r/31070/diff/3/?file=888609#file888609line2162>
> >
> > It would be useful to provide an option of merging and overwritting the existing roles
done.
> On May 11, 2015, 9:15 p.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 2225
> > <https://reviews.apache.org/r/31070/diff/3/?file=888609#file888609line2225>
> >
> > Should it handle merging of privilege, eg if you already have 'all' or appending 'all' then the others can be discarded.
Update the import logical, the exist method alterSentryRoleGrantPrivilegeCore() is used for import privileges.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31070/#review83272
-----------------------------------------------------------
On March 9, 2015, 2:49 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31070/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 2:49 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 136dab6
> 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
>
>