You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Gregory Chanan <gc...@cloudera.com> on 2016/02/03 02:58:50 UTC
Review Request 43130: SENTRY-1047: Use existing validators in
SentryShellSolr
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43130/
-----------------------------------------------------------
Review request for sentry, Colin Ma, Lenni Kuff, and Sravya Tirukkovalur.
Repository: sentry
Description
-------
Right now, following the pattern of the Hive shell, the Solr shell manually implements validators, i.e.:
https://github.com/apache/incubator-sentry/blob/597a3cdd319be84f2417c96d24db01553f264551/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java#L115-L130
It would be better if we automatically used the existing validators, so they don't need to be implemented in multiple places.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java e2b01a45a32d9f60a6f2d596e5149ca4a8d0188c
Diff: https://reviews.apache.org/r/43130/diff/
Testing
-------
Ran unit tests.
Thanks,
Gregory Chanan
Re: Review Request 43130: SENTRY-1047: Use existing validators in
SentryShellSolr
Posted by Gregory Chanan <gc...@cloudera.com>.
> On Feb. 3, 2016, 7:08 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java, line 119
> > <https://reviews.apache.org/r/43130/diff/1/?file=1230319#file1230319line119>
> >
> > Add TODO to cleanup validator duplication and file JIRA to track it?
>
> Gregory Chanan wrote:
> I'm not sure what you mean. This avoids the duplication code in the shell, which previously basically copied the validation code (you can check https://github.com/apache/incubator-sentry/blob/master/sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/CollectionRequiredInPrivilege.java for the code the previous function basically duplication). Do you mean file a jira for the Hive shell?
>
> Lenni Kuff wrote:
> Sorry if I wasn't clear, I meant to add a TODO around automatically using the existing validators
I filed https://issues.apache.org/jira/browse/SENTRY-1047 for doing this automatically with other services, including hive.
- Gregory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43130/#review117579
-----------------------------------------------------------
On Feb. 3, 2016, 1:58 a.m., Gregory Chanan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43130/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 1:58 a.m.)
>
>
> Review request for sentry, Colin Ma, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, following the pattern of the Hive shell, the Solr shell manually implements validators, i.e.:
> https://github.com/apache/incubator-sentry/blob/597a3cdd319be84f2417c96d24db01553f264551/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java#L115-L130
>
> It would be better if we automatically used the existing validators, so they don't need to be implemented in multiple places.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java e2b01a45a32d9f60a6f2d596e5149ca4a8d0188c
>
> Diff: https://reviews.apache.org/r/43130/diff/
>
>
> Testing
> -------
>
> Ran unit tests.
>
>
> Thanks,
>
> Gregory Chanan
>
>
Re: Review Request 43130: SENTRY-1047: Use existing validators in
SentryShellSolr
Posted by Gregory Chanan <gc...@cloudera.com>.
> On Feb. 3, 2016, 7:08 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java, line 119
> > <https://reviews.apache.org/r/43130/diff/1/?file=1230319#file1230319line119>
> >
> > Add TODO to cleanup validator duplication and file JIRA to track it?
I'm not sure what you mean. This avoids the duplication code in the shell, which previously basically copied the validation code (you can check https://github.com/apache/incubator-sentry/blob/master/sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/CollectionRequiredInPrivilege.java for the code the previous function basically duplication). Do you mean file a jira for the Hive shell?
- Gregory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43130/#review117579
-----------------------------------------------------------
On Feb. 3, 2016, 1:58 a.m., Gregory Chanan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43130/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 1:58 a.m.)
>
>
> Review request for sentry, Colin Ma, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, following the pattern of the Hive shell, the Solr shell manually implements validators, i.e.:
> https://github.com/apache/incubator-sentry/blob/597a3cdd319be84f2417c96d24db01553f264551/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java#L115-L130
>
> It would be better if we automatically used the existing validators, so they don't need to be implemented in multiple places.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java e2b01a45a32d9f60a6f2d596e5149ca4a8d0188c
>
> Diff: https://reviews.apache.org/r/43130/diff/
>
>
> Testing
> -------
>
> Ran unit tests.
>
>
> Thanks,
>
> Gregory Chanan
>
>
Re: Review Request 43130: SENTRY-1047: Use existing validators in
SentryShellSolr
Posted by Lenni Kuff <ls...@cloudera.com>.
> On Feb. 3, 2016, 7:08 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java, line 119
> > <https://reviews.apache.org/r/43130/diff/1/?file=1230319#file1230319line119>
> >
> > Add TODO to cleanup validator duplication and file JIRA to track it?
>
> Gregory Chanan wrote:
> I'm not sure what you mean. This avoids the duplication code in the shell, which previously basically copied the validation code (you can check https://github.com/apache/incubator-sentry/blob/master/sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/CollectionRequiredInPrivilege.java for the code the previous function basically duplication). Do you mean file a jira for the Hive shell?
Sorry if I wasn't clear, I meant to add a TODO around automatically using the existing validators
- Lenni
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43130/#review117579
-----------------------------------------------------------
On Feb. 3, 2016, 1:58 a.m., Gregory Chanan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43130/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 1:58 a.m.)
>
>
> Review request for sentry, Colin Ma, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, following the pattern of the Hive shell, the Solr shell manually implements validators, i.e.:
> https://github.com/apache/incubator-sentry/blob/597a3cdd319be84f2417c96d24db01553f264551/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java#L115-L130
>
> It would be better if we automatically used the existing validators, so they don't need to be implemented in multiple places.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java e2b01a45a32d9f60a6f2d596e5149ca4a8d0188c
>
> Diff: https://reviews.apache.org/r/43130/diff/
>
>
> Testing
> -------
>
> Ran unit tests.
>
>
> Thanks,
>
> Gregory Chanan
>
>
Re: Review Request 43130: SENTRY-1047: Use existing validators in
SentryShellSolr
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43130/#review117579
-----------------------------------------------------------
Fix it, then Ship it!
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java (line 117)
<https://reviews.apache.org/r/43130/#comment178817>
Add TODO to cleanup validator duplication and file JIRA to track it?
- Lenni Kuff
On Feb. 3, 2016, 1:58 a.m., Gregory Chanan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43130/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 1:58 a.m.)
>
>
> Review request for sentry, Colin Ma, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, following the pattern of the Hive shell, the Solr shell manually implements validators, i.e.:
> https://github.com/apache/incubator-sentry/blob/597a3cdd319be84f2417c96d24db01553f264551/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java#L115-L130
>
> It would be better if we automatically used the existing validators, so they don't need to be implemented in multiple places.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java e2b01a45a32d9f60a6f2d596e5149ca4a8d0188c
>
> Diff: https://reviews.apache.org/r/43130/diff/
>
>
> Testing
> -------
>
> Ran unit tests.
>
>
> Thanks,
>
> Gregory Chanan
>
>