You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Colin Ma <ju...@intel.com> on 2015/05/12 09:11:02 UTC
Review Request 34086: SENTRY-734: Update SentryPolicyStoreProcessor
for grant user to role
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/
-----------------------------------------------------------
Review request for sentry, Dapeng Sun, shen guoquan, and Prasad Mujumdar.
Repository: sentry
Description
-------
Update SentryPolicyStoreProcessor for grant user to role
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 7a9f0df
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 44681ca
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 30792f3
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 02c7535
Diff: https://reviews.apache.org/r/34086/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Colin Ma <ju...@intel.com>.
> On 四月 8, 2016, 4:59 a.m., Jerry Chen wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java, line 65
> > <https://reviews.apache.org/r/34086/diff/4/?file=1332107#file1332107line65>
> >
> > If we don't perform any grantRoleToUser operation in the test case, this usage of users is not so meaningful. The same with other cases following that we pass requtorUserNames.
> >
> > If we want to use new cases to cover this and don't want to modify here, I suggest to still use null.
I'm agreed with you, thank you.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/#review127738
-----------------------------------------------------------
On 四月 8, 2016, 4:26 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34086/
> -----------------------------------------------------------
>
> (Updated 四月 8, 2016, 4:26 a.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryPolicyStoreProcessor for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
>
> Diff: https://reviews.apache.org/r/34086/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Jerry Chen <ha...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/#review127738
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java (line 65)
<https://reviews.apache.org/r/34086/#comment191125>
If we don't perform any grantRoleToUser operation in the test case, this usage of users is not so meaningful. The same with other cases following that we pass requtorUserNames.
If we want to use new cases to cover this and don't want to modify here, I suggest to still use null.
- Jerry Chen
On April 8, 2016, 4:26 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34086/
> -----------------------------------------------------------
>
> (Updated April 8, 2016, 4:26 a.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryPolicyStoreProcessor for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
>
> Diff: https://reviews.apache.org/r/34086/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Colin Ma <ju...@intel.com>.
> On 四月 8, 2016, 7:55 a.m., Jerry Chen wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, line 694
> > <https://reviews.apache.org/r/34086/diff/5/?file=1332223#file1332223line694>
> >
> > In concept, we may don't need to list the roles for the groups of the user.
> >
> > The command is to list the roles directly assigned to the user.
Agree, also update https://reviews.apache.org/r/34079/
> On 四月 8, 2016, 7:55 a.m., Jerry Chen wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, line 680
> > <https://reviews.apache.org/r/34086/diff/5/?file=1332223#file1332223line680>
> >
> > I saw the similar method for group, there is client version check as following:
> >
> > validateClientVersion(request.getProtocol_version());
> >
> > Is this necessary for this method?
Thank you for your comments, I have fixed it.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/#review127748
-----------------------------------------------------------
On 四月 8, 2016, 9:20 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34086/
> -----------------------------------------------------------
>
> (Updated 四月 8, 2016, 9:20 a.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryPolicyStoreProcessor for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
>
> Diff: https://reviews.apache.org/r/34086/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Jerry Chen <ha...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/#review127748
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 506)
<https://reviews.apache.org/r/34086/#comment191136>
validateClientVersion(request.getProtocol_version()); is needed
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 543)
<https://reviews.apache.org/r/34086/#comment191137>
validateClientVersion(request.getProtocol_version());
is needed.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 680)
<https://reviews.apache.org/r/34086/#comment191134>
I saw the similar method for group, there is client version check as following:
validateClientVersion(request.getProtocol_version());
Is this necessary for this method?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 694)
<https://reviews.apache.org/r/34086/#comment191135>
In concept, we may don't need to list the roles for the groups of the user.
The command is to list the roles directly assigned to the user.
- Jerry Chen
On April 8, 2016, 6:40 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34086/
> -----------------------------------------------------------
>
> (Updated April 8, 2016, 6:40 a.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryPolicyStoreProcessor for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
>
> Diff: https://reviews.apache.org/r/34086/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/#review128323
-----------------------------------------------------------
Ship it!
Ship It!
- Dapeng Sun
On 四月 11, 2016, 2:20 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34086/
> -----------------------------------------------------------
>
> (Updated 四月 11, 2016, 2:20 p.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryPolicyStoreProcessor for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
>
> Diff: https://reviews.apache.org/r/34086/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Jerry Chen <ha...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/#review128081
-----------------------------------------------------------
LGTM
- Jerry Chen
On April 11, 2016, 6:20 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34086/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 6:20 a.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryPolicyStoreProcessor for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
>
> Diff: https://reviews.apache.org/r/34086/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/
-----------------------------------------------------------
(Updated 四月 11, 2016, 6:20 a.m.)
Review request for sentry, Dapeng Sun and Jerry Chen.
Repository: sentry
Description
-------
Update SentryPolicyStoreProcessor for grant user to role
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
Diff: https://reviews.apache.org/r/34086/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/
-----------------------------------------------------------
(Updated 四月 8, 2016, 9:20 a.m.)
Review request for sentry, Dapeng Sun and Jerry Chen.
Repository: sentry
Description
-------
Update SentryPolicyStoreProcessor for grant user to role
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
Diff: https://reviews.apache.org/r/34086/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/
-----------------------------------------------------------
(Updated 四月 8, 2016, 6:40 a.m.)
Review request for sentry, Dapeng Sun and Jerry Chen.
Repository: sentry
Description
-------
Update SentryPolicyStoreProcessor for grant user to role
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
Diff: https://reviews.apache.org/r/34086/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/
-----------------------------------------------------------
(Updated 四月 8, 2016, 4:26 a.m.)
Review request for sentry, Dapeng Sun and Jerry Chen.
Repository: sentry
Description
-------
Update SentryPolicyStoreProcessor for grant user to role
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
Diff: https://reviews.apache.org/r/34086/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Colin Ma <ju...@intel.com>.
> On 四月 7, 2016, 6:17 a.m., Jerry Chen wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, line 798
> > <https://reviews.apache.org/r/34086/diff/3/?file=1327604#file1327604line798>
> >
> > Question: why we need to remove the exception handler?
Thank you for catching it.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/#review127564
-----------------------------------------------------------
On 四月 6, 2016, 2:56 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34086/
> -----------------------------------------------------------
>
> (Updated 四月 6, 2016, 2:56 p.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryPolicyStoreProcessor for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
>
> Diff: https://reviews.apache.org/r/34086/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Jerry Chen <ha...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/#review127564
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
<https://reviews.apache.org/r/34086/#comment190942>
Question: why we need to remove the exception handler?
- Jerry Chen
On April 6, 2016, 2:56 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34086/
> -----------------------------------------------------------
>
> (Updated April 6, 2016, 2:56 p.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryPolicyStoreProcessor for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
>
> Diff: https://reviews.apache.org/r/34086/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Colin Ma <ju...@intel.com>.
> On 四月 7, 2016, 5:44 a.m., Jerry Chen wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java, line 107
> > <https://reviews.apache.org/r/34086/diff/3/?file=1327605#file1327605line107>
> >
> > Use null instead of new HashSet<String>(). The same for the following instances.
> >
> > One question to consider: Is the original version of listPrivilegesForProvider without users parameters still be meaningful?
> >
> > If the answer is YES, then we can add an overload version with users instead of directly changing the original one with users parameter.
Thank Jerry for your comments, I have update all the set with using ADMIN_USER to construct the userNames.
`Is the original version of listPrivilegesForProvider without users parameters still be meaningful?`
I think it won't meaningful. `listPrivilegesForProvider` is only used for `SimpleDBProviderBackend`, the provider have been updated with added users parameters.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/#review127555
-----------------------------------------------------------
On 四月 6, 2016, 2:56 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34086/
> -----------------------------------------------------------
>
> (Updated 四月 6, 2016, 2:56 p.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryPolicyStoreProcessor for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
>
> Diff: https://reviews.apache.org/r/34086/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Jerry Chen <ha...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/#review127555
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java (line 107)
<https://reviews.apache.org/r/34086/#comment190918>
Use null instead of new HashSet<String>(). The same for the following instances.
One question to consider: Is the original version of listPrivilegesForProvider without users parameters still be meaningful?
If the answer is YES, then we can add an overload version with users instead of directly changing the original one with users parameter.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java (line 134)
<https://reviews.apache.org/r/34086/#comment190920>
Can we also use ADMIN_USER to construct the userNames.
requestorUserNames = Sets.newHashSet(ADMIN_USER);
And perform grantRoleToUser similar to group to test both groups and users in this case instead of passing the empty user list.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java (line 140)
<https://reviews.apache.org/r/34086/#comment190921>
Can we also use ADMIN_USER to construct the userNames.
requestorUserNames = Sets.newHashSet(ADMIN_USER);
And perform grantRoleToUser similar to group to test both groups and users in this case instead of passing the empty user list.
- Jerry Chen
On April 6, 2016, 2:56 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34086/
> -----------------------------------------------------------
>
> (Updated April 6, 2016, 2:56 p.m.)
>
>
> Review request for sentry, Dapeng Sun and Jerry Chen.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryPolicyStoreProcessor for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
>
> Diff: https://reviews.apache.org/r/34086/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/
-----------------------------------------------------------
(Updated 四月 6, 2016, 2:56 p.m.)
Review request for sentry, Dapeng Sun and Jerry Chen.
Repository: sentry
Description
-------
Update SentryPolicyStoreProcessor for grant user to role
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
Diff: https://reviews.apache.org/r/34086/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/
-----------------------------------------------------------
(Updated 四月 6, 2016, 5:30 a.m.)
Review request for sentry, Dapeng Sun, shen guoquan, and Prasad Mujumdar.
Changes
-------
Update patch
Repository: sentry
Description
-------
Update SentryPolicyStoreProcessor for grant user to role
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java de50adb
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java edc5661
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8881d82
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java ac4df77
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 0792eb6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 07c7f7a
Diff: https://reviews.apache.org/r/34086/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 34086: SENTRY-734: Update
SentryPolicyStoreProcessor for grant user to role
Posted by Jerry Chen <ha...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34086/#review126529
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java (line 145)
<https://reviews.apache.org/r/34086/#comment189550>
Can the name the same with listPrivilegesForProvider?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java (line 240)
<https://reviews.apache.org/r/34086/#comment189551>
Trailing white space. Please remove.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 692)
<https://reviews.apache.org/r/34086/#comment189555>
Use static empty set instance for utils or define by you own.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 726)
<https://reviews.apache.org/r/34086/#comment189556>
The same as to the name "ug".
- Jerry Chen
On May 12, 2015, 7:11 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34086/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 7:11 a.m.)
>
>
> Review request for sentry, Dapeng Sun, shen guoquan, and Prasad Mujumdar.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Update SentryPolicyStoreProcessor for grant user to role
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 7a9f0df
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java 44681ca
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 30792f3
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 02c7535
>
> Diff: https://reviews.apache.org/r/34086/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>