You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sentry.apache.org by shen guoquan <gu...@intel.com> on 2014/12/19 06:54:38 UTC

Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/
-----------------------------------------------------------

Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.


Repository: sentry


Description
-------

Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
1. Can't satisfied with the needs of dynamically add, delete and update permissions
2. Can't be centrally managed and difficult to maintain
According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).


Diffs
-----

  sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
  sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java PRE-CREATION 
  sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
  sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java PRE-CREATION 
  sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/conf/SolrAuthzConf.java 227f75e 
  sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
  sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
  sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 

Diff: https://reviews.apache.org/r/29238/diff/


Testing
-------


Thanks,

shen guoquan

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.


> On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote:
> > sentry-binding/sentry-binding-solr/pom.xml, line 56
> > <https://reviews.apache.org/r/29238/diff/1/?file=797085#file797085line56>
> >
> >     Why can this no longer just be in test scope?

Yeah, I will fix it


> On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote:
> > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java, line 17
> > <https://reviews.apache.org/r/29238/diff/1/?file=797086#file797086line17>
> >
> >     This doesn't seem like the correct location; all the other ProviderBackends are under /sentry-provider

I agree with your opinion.


> On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote:
> > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java, line 76
> > <https://reviews.apache.org/r/29238/diff/1/?file=797086#file797086line76>
> >
> >     We don't use the active role set here at all?

I am sorry about making a mistake here. I will fix it. Thanks your feedback.


> On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote:
> > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java, line 40
> > <https://reviews.apache.org/r/29238/diff/1/?file=797086#file797086line40>
> >
> >     final?

Thanks for your comments, I will fix it.


> On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote:
> > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java, line 65
> > <https://reviews.apache.org/r/29238/diff/1/?file=797088#file797088line65>
> >
> >     do we care that this client is completely synchronous?  Performance problem?

Yeah, this client is completely synchronous, if the keyword synchronized removed, the thift MetaException(out of sequence response) was throwed. For example, when one request has been send to service, the client was waiting for response froms service, because the client is not synchronous, another thread using the client send a request when the previous request hasn't finished, then the out of sequence exception will occurred.


> On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote:
> > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/conf/SolrAuthzConf.java, line 37
> > <https://reviews.apache.org/r/29238/diff/1/?file=797089#file797089line37>
> >
> >     isn't this the service name, not the server name?

The property of sentry.solr.server was used to distinct itself from multiple solr clusters. For example, there are two
solr cluster server1 and server2 authorized through sentry, it must set the value of sentry.solr.server=server1 or server2
to communicate with sentry service for authorization


> On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote:
> > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java, line 230
> > <https://reviews.apache.org/r/29238/diff/1/?file=797088#file797088line230>
> >
> >     I still don't get why this is called requestorUserName.  I want to get the roles for a certain user, whether that is the requestor or not seems orthogonal and something that should be handled by the authenticaiton layer.

May be there is a mistake I have. Changing the requestorUserName to user is better and can understand easier.


> On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java, line 55
> > <https://reviews.apache.org/r/29238/diff/1/?file=797091#file797091line55>
> >
> >     Can we add some documentation here?  What is the requestor param for?

Hi chanan. I guess you have a confussion about this requestor param. I want to explain it. At first, the sentry service exposes a thrift API list_sentry_roles_by_group(string requestor, string groupName, ActiveRoleSet roleSet). The logic of this function as followings:
if (groupName == null){
  //list all role in the sentry, The requestor must be belongs to adminGroup
  Set<string> requestorGroups = getGroups(requestor);
  if (requestorGroups has intersection with adminGroups) {
     roles = store.getAllRoles();
     return roles
  }else {
   throw denied exception
  }
  
} else if (groupName == "ALL") {
  //list roles belongs to requestor
  Set<string> requestorGroups = getGroups(requestor);
  roles = store.getRoles(requestorGroups)
  return roles
} else {
  //list roles according to the groupName, the requestor must be belongs to adminGroup or belong to the request groupName
  Set<string> requestorGroups = getGroups(requestor);
  if ((requestorGroups has intersection with adminGroups) or (requestorGroups.contain(groupName))){
    roles = store.getRoles(groupName);
    return roles;
  } else {
    throw denied exception
  }
}

That's the procedure of list_roles_by_groups in the sentry service side. The Hive authorization with Sentry also using this thrift API to support commands like "show roles", the requestor will be gotten from UGI. When the solr integration with
DB store, it must be communicate with snetry service. when solr want to get roles by group, it must transfer the requestor to sentry serivce side. the requestor can be gotten from SolrHadoopAuthenticationFilter. So the requestor parameter must be add.


> On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote:
> > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java, line 71
> > <https://reviews.apache.org/r/29238/diff/1/?file=797088#file797088line71>
> >
> >     this isn't accureate, we don't renew the client if it's already been renewed.

Currently Solr integrates authorization with sentry using the singleton class SentryIndexAuthorizationSingleton. The SentryIndexAuthorizationSingleton will be intialized when it was loaded, so the SolrAuthzBinding in the SentryIndexAuthorizationSingleton also has been intialized when the SentryIndexAuthorizationSingleton was loaded. When the solr integrated with db store, the SearchProviderBackend was created in the SolrAuthzBinding and using the SolrSentryRetryClient to communicate with sentry service. Because the SolrAuthzBinding has no chance to intialized again, the SolrSentryRetryClient in the 
SearchProviderBackend will not reconnect to the sentry serivce. There is a problem. when the Sentry service restart, the client will throw a thrift connection exception when sending requests to Sentry service because it has already restarted, the client must has a ability to reconnect to the service.


- shen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review65823
-----------------------------------------------------------


On 十二月 19, 2014, 5:54 a.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated 十二月 19, 2014, 5:54 a.m.)
> 
> 
> Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java PRE-CREATION 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java PRE-CREATION 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/conf/SolrAuthzConf.java 227f75e 
>   sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
>   sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by Gregory Chanan <gc...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review65823
-----------------------------------------------------------



sentry-binding/sentry-binding-solr/pom.xml
<https://reviews.apache.org/r/29238/#comment109071>

    Why can this no longer just be in test scope?



sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java
<https://reviews.apache.org/r/29238/#comment109062>

    This doesn't seem like the correct location; all the other ProviderBackends are under /sentry-provider



sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java
<https://reviews.apache.org/r/29238/#comment109066>

    On the name of the class..
    
    Is there anything that is specific to Search here?
    
    Also, DB refers to the service backed by the DB?  Should that be named based on Service rather than DB, since there is also a DB model?



sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java
<https://reviews.apache.org/r/29238/#comment109064>

    final?



sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java
<https://reviews.apache.org/r/29238/#comment109069>

    We don't use the active role set here at all?



sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java
<https://reviews.apache.org/r/29238/#comment109067>

    we don't validate the policy or throw an exception saying we aren't validating?



sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java
<https://reviews.apache.org/r/29238/#comment109079>

    do we care that this client is completely synchronous?  Performance problem?



sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java
<https://reviews.apache.org/r/29238/#comment109075>

    this isn't accureate, we don't renew the client if it's already been renewed.



sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java
<https://reviews.apache.org/r/29238/#comment109074>

    Error connecting to the sentry service.  Renewing the client and trying again



sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java
<https://reviews.apache.org/r/29238/#comment109080>

    I still don't get why this is called requestorUserName.  I want to get the roles for a certain user, whether that is the requestor or not seems orthogonal and something that should be handled by the authenticaiton layer.



sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/conf/SolrAuthzConf.java
<https://reviews.apache.org/r/29238/#comment109072>

    isn't this the service name, not the server name?



sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java
<https://reviews.apache.org/r/29238/#comment109060>

    Can we add some documentation here?  What is the requestor param for?


- Gregory Chanan


On Dec. 19, 2014, 5:54 a.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated Dec. 19, 2014, 5:54 a.m.)
> 
> 
> Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java PRE-CREATION 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java PRE-CREATION 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/conf/SolrAuthzConf.java 227f75e 
>   sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
>   sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.


> On 一月 28, 2015, 6:31 a.m., Colin Ma wrote:
> > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java, line 29
> > <https://reviews.apache.org/r/29238/diff/3/?file=816327#file816327line29>
> >
> >     The constant is for configuration, this should be added to org.apache.sentry.service.thrift.ServiceConstants.

This configuration was used in the solr component. I think the search model package is a better place than provider-db package


> On 一月 28, 2015, 6:31 a.m., Colin Ma wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 65
> > <https://reviews.apache.org/r/29238/diff/3/?file=816332#file816332line65>
> >
> >     Currently, sentry won't retry the operation, if connection is broken, just throw the exception.

There should be deal with the situation that the sentry service is restarted. This is a simple solution. I will refactor the client when the connection pool was committed.


> On 一月 28, 2015, 6:31 a.m., Colin Ma wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 61
> > <https://reviews.apache.org/r/29238/diff/3/?file=816332#file816332line61>
> >
> >     This interface is not necessary.

There should be deal with the situation that the sentry service is restarted. This is a simple solution. I will refactor the client when the connection pool was committed.


> On 一月 28, 2015, 6:31 a.m., Colin Ma wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 89
> > <https://reviews.apache.org/r/29238/diff/3/?file=816332#file816332line89>
> >
> >     The SearchPolicyServiceClient shouldn't retry the operation currently, so renewClient() is not necessary.

The reason is above.


> On 一月 28, 2015, 6:31 a.m., Colin Ma wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 108
> > <https://reviews.apache.org/r/29238/diff/3/?file=816332#file816332line108>
> >
> >     Confuse on ifExist method, too.....

This method is mainly used in the import tool. When the policy file want to import the sentry service, this method is very useful


> On 一月 28, 2015, 6:31 a.m., Colin Ma wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 49
> > <https://reviews.apache.org/r/29238/diff/3/?file=816332#file816332line49>
> >
> >     The value should be in lowercase as the roleName. The variable name should be COMPONENT_TYPE_SOLR, and these kind of constants should be put in one class including all component_type, eg:
> >       COMPONENT_TYPE_SOLR = "solr";
> >       COMPONENT_TYPE_SQOOP = "sqoop";
> >       .......

Thanks for your advice. I will fix it


- shen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review69962
-----------------------------------------------------------


On 一月 12, 2015, 9:10 a.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated 一月 12, 2015, 9:10 a.m.)
> 
> 
> Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
>   sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by Colin Ma <ju...@intel.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review69962
-----------------------------------------------------------



sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java
<https://reviews.apache.org/r/29238/#comment114804>

    The constant is for configuration, this should be added to org.apache.sentry.service.thrift.ServiceConstants.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment114807>

    The value should be in lowercase as the roleName. The variable name should be COMPONENT_TYPE_SOLR, and these kind of constants should be put in one class including all component_type, eg:
      COMPONENT_TYPE_SOLR = "solr";
      COMPONENT_TYPE_SQOOP = "sqoop";
      .......



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment114806>

    This interface is not necessary.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment114805>

    Currently, sentry won't retry the operation, if connection is broken, just throw the exception.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment114808>

    The SearchPolicyServiceClient shouldn't retry the operation currently, so renewClient() is not necessary.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment114811>

    Confuse on ifExist method, too.....


- Colin Ma


On Jan. 12, 2015, 9:10 a.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 9:10 a.m.)
> 
> 
> Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
>   sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.


> On 一月 28, 2015, 3:30 a.m., Xiaomeng Huang wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 65
> > <https://reviews.apache.org/r/29238/diff/3/?file=816332#file816332line65>
> >
> >     why need synchronized? this client will be shared by multiple thread or process?

If the keyword synchronized is not added, the sentry service will throw out of sequence exception


> On 一月 28, 2015, 3:30 a.m., Xiaomeng Huang wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 96
> > <https://reviews.apache.org/r/29238/diff/3/?file=816332#file816332line96>
> >
> >     It seems if we don't do renew operation, we don't need write so complex, one line code "client.createRole(requestor, roleName, SOLR)" is enough. Even this SearchPolicyServiceClient is not needed. Could explain why you do renew?

There should be deal with the situation that the sentry service is restarted. This is a simple solution. I will refactor the client when the connection pool was committed.


> On 一月 28, 2015, 3:30 a.m., Xiaomeng Huang wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 114
> > <https://reviews.apache.org/r/29238/diff/3/?file=816332#file816332line114>
> >
> >     additional blank line

Thanks.


> On 一月 28, 2015, 3:30 a.m., Xiaomeng Huang wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 108
> > <https://reviews.apache.org/r/29238/diff/3/?file=816332#file816332line108>
> >
> >     why we need createRoleIfNotExist? createRole is not enough?

This method is mainly used in the import tool. When the policy file want to import the sentry service, this method is very useful


- shen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review69943
-----------------------------------------------------------


On 一月 12, 2015, 9:10 a.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated 一月 12, 2015, 9:10 a.m.)
> 
> 
> Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
>   sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by Xiaomeng Huang <xi...@intel.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review69943
-----------------------------------------------------------



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment114786>

    why need synchronized? this client will be shared by multiple thread or process?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment114788>

    It seems if we don't do renew operation, we don't need write so complex, one line code "client.createRole(requestor, roleName, SOLR)" is enough. Even this SearchPolicyServiceClient is not needed. Could explain why you do renew?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment114783>

    why we need createRoleIfNotExist? createRole is not enough?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment114785>

    additional blank line


- Xiaomeng Huang


On 一月 12, 2015, 9:10 a.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated 一月 12, 2015, 9:10 a.m.)
> 
> 
> Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
>   sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.


> On 一月 28, 2015, 3:25 a.m., Dapeng Sun wrote:
> > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java, line 147
> > <https://reviews.apache.org/r/29238/diff/3/?file=816326#file816326line147>
> >
> >     Why use **user** as requestor

Thanks for your question. I will delete this requestor. Thanks.


> On 一月 28, 2015, 3:25 a.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java, line 55
> > <https://reviews.apache.org/r/29238/diff/3/?file=816330#file816330line55>
> >
> >     It seems a interface in common, may be used by HIVE, May I know why you make it change?

Yeah. I agree with you. The common interface shouldn't been changed.


> On 一月 28, 2015, 3:25 a.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 61
> > <https://reviews.apache.org/r/29238/diff/3/?file=816332#file816332line61>
> >
> >     Retries in **SearchPolicyServiceClient** might be confused. **SearchPolicyServiceClient** should be simple.

There should be deal with the situation that the sentry service is restarted. This is a simple solution. I will refactor the client when the connection pool was committed.


> On 一月 28, 2015, 3:25 a.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java, line 61
> > <https://reviews.apache.org/r/29238/diff/3/?file=816333#file816333line61>
> >
> >     Why not move the retries here?

There should not add the retry mechanism.


- shen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review69947
-----------------------------------------------------------


On 一月 12, 2015, 9:10 a.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated 一月 12, 2015, 9:10 a.m.)
> 
> 
> Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
>   sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by Dapeng Sun <da...@intel.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review69947
-----------------------------------------------------------



sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java
<https://reviews.apache.org/r/29238/#comment114781>

    Why use **user** as requestor



sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java
<https://reviews.apache.org/r/29238/#comment114782>

    It seems a interface in common, may be used by HIVE, May I know why you make it change?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment114784>

    Retries in **SearchPolicyServiceClient** might be confused. **SearchPolicyServiceClient** should be simple.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java
<https://reviews.apache.org/r/29238/#comment114787>

    Why not move the retries here?


- Dapeng Sun


On 一月 12, 2015, 5:10 p.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated 一月 12, 2015, 5:10 p.m.)
> 
> 
> Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
>   sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/
-----------------------------------------------------------

(Updated 二月 9, 2015, 2:32 a.m.)


Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, and Vamsee Yarlagadda.


Repository: sentry


Description
-------

Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
1. Can't satisfied with the needs of dynamically add, delete and update permissions
2. Can't be centrally managed and difficult to maintain
According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).


Diffs (updated)
-----

  sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
  sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
  sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
  sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29238/diff/


Testing
-------

All tests have passed through the local jenkins environment.


Thanks,

shen guoquan

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.


> On 二月 7, 2015, 1:40 a.m., Lenni Kuff wrote:
> >

Hi Lenni. Thanks for your review. I will update the newset patch as soon as possible. Please help look at it. If you have no more comment, can you give the jira sentry-478 plus one. Thanks for your help


> On 二月 7, 2015, 1:40 a.m., Lenni Kuff wrote:
> > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java, line 25
> > <https://reviews.apache.org/r/29238/diff/6/?file=841302#file841302line25>
> >
> >     used to distinguish...

Thanks Lenni. I will fix it.


> On 二月 7, 2015, 1:40 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java, line 19
> > <https://reviews.apache.org/r/29238/diff/6/?file=841304#file841304line19>
> >
> >     Grammar. Maybe "Component being authorized by Sentry"? 
> >     
> >     Also, HIVE is very specific (it excludes Impala). Perhaps "SQL" or "DB"

Thanks Lenni for your advice. I will fix it.


> On 二月 7, 2015, 1:40 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java, line 38
> > <https://reviews.apache.org/r/29238/diff/6/?file=841306#file841306line38>
> >
> >     Add class comment.

It's my mistake. I will fix it . Thanks for your comment.


> On 二月 7, 2015, 1:40 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java, line 57
> > <https://reviews.apache.org/r/29238/diff/6/?file=841306#file841306line57>
> >
> >     Would it make this easier to use if you could call initialize multiple times?

hi lenni. I think the initialize function can't be called multiple times. I use the same logic as the hive component backend provider. Thanks for your comment.


- shen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review71535
-----------------------------------------------------------


On 一月 30, 2015, 5:55 a.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated 一月 30, 2015, 5:55 a.m.)
> 
> 
> Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> All tests have passed through the local jenkins environment.
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by Lenni Kuff <ls...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review71535
-----------------------------------------------------------

Ship it!



sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java
<https://reviews.apache.org/r/29238/#comment117276>

    used to distinguish...



sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java
<https://reviews.apache.org/r/29238/#comment117277>

    Grammar. Maybe "Component being authorized by Sentry"? 
    
    Also, HIVE is very specific (it excludes Impala). Perhaps "SQL" or "DB"



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment117278>

    Why do you have an enum for AuthorizationComponent when you are using it like a String? Either change AithorizationComponent to be string constants or make this an enum.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java
<https://reviews.apache.org/r/29238/#comment117279>

    Add class comment.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java
<https://reviews.apache.org/r/29238/#comment117280>

    Would it make this easier to use if you could call initialize multiple times?


- Lenni Kuff


On Jan. 30, 2015, 5:55 a.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2015, 5:55 a.m.)
> 
> 
> Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> All tests have passed through the local jenkins environment.
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by Dapeng Sun <da...@intel.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review70317
-----------------------------------------------------------

Ship it!


Ship It!

- Dapeng Sun


On 一月 30, 2015, 1:55 p.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated 一月 30, 2015, 1:55 p.m.)
> 
> 
> Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> All tests have passed through the local jenkins environment.
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/
-----------------------------------------------------------

(Updated 一月 30, 2015, 5:55 a.m.)


Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, and Vamsee Yarlagadda.


Repository: sentry


Description
-------

Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
1. Can't satisfied with the needs of dynamically add, delete and update permissions
2. Can't be centrally managed and difficult to maintain
According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).


Diffs (updated)
-----

  sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
  sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
  sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
  sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29238/diff/


Testing
-------

All tests have passed through the local jenkins environment.


Thanks,

shen guoquan

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/
-----------------------------------------------------------

(Updated 一月 30, 2015, 5:36 a.m.)


Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, and Vamsee Yarlagadda.


Repository: sentry


Description
-------

Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
1. Can't satisfied with the needs of dynamically add, delete and update permissions
2. Can't be centrally managed and difficult to maintain
According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).


Diffs (updated)
-----

  sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
  sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
  sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
  sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29238/diff/


Testing
-------

All tests have passed through the local jenkins environment.


Thanks,

shen guoquan

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.


> On 一月 29, 2015, 7:52 a.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java, line 47
> > <https://reviews.apache.org/r/29238/diff/4/?file=839738#file839738line47>
> >
> >     Have you do any test about it in kerberos?

Yes, I have test it in the e2e test. Thanks Dapeng


> On 一月 29, 2015, 7:52 a.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 48
> > <https://reviews.apache.org/r/29238/diff/4/?file=839737#file839737line48>
> >
> >     Only two issues for me, one is **synchronized**, another is **reties**.
> >     I'm agree with you that Connection Pool will help to resolve two issues, a workaround will be create a **SearchPolicyServiceClient** for every request.

Thanks for your advice, Dapeng. I agree with you. I will fix it.


> On 一月 29, 2015, 7:52 a.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java, line 66
> > <https://reviews.apache.org/r/29238/diff/4/?file=839738#file839738line66>
> >
> >     Why not create a new **SearchPolicyServiceClient** for every request here

Thanks for your advice, Dapeng. I agree with you. I will fix it.


- shen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review70173
-----------------------------------------------------------


On 一月 29, 2015, 7:04 a.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated 一月 29, 2015, 7:04 a.m.)
> 
> 
> Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> All tests have passed through the local jenkins environment.
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by Dapeng Sun <da...@intel.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review70173
-----------------------------------------------------------


Hi Guoquan, great work. Most original issues of mine have resolved.


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment115218>

    Only two issues for me, one is **synchronized**, another is **reties**.
    I'm agree with you that Connection Pool will help to resolve two issues, a workaround will be create a **SearchPolicyServiceClient** for every request.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java
<https://reviews.apache.org/r/29238/#comment115212>

    Have you do any test about it in kerberos?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java
<https://reviews.apache.org/r/29238/#comment115219>

    Why not create a new **SearchPolicyServiceClient** for every request here


- Dapeng Sun


On 一月 29, 2015, 3:04 p.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated 一月 29, 2015, 3:04 p.m.)
> 
> 
> Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> All tests have passed through the local jenkins environment.
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/
-----------------------------------------------------------

(Updated 一月 29, 2015, 7:04 a.m.)


Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, and Vamsee Yarlagadda.


Repository: sentry


Description
-------

Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
1. Can't satisfied with the needs of dynamically add, delete and update permissions
2. Can't be centrally managed and difficult to maintain
According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).


Diffs (updated)
-----

  sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
  sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
  sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
  sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29238/diff/


Testing (updated)
-------

All tests have passed through the local jenkins environment.


Thanks,

shen guoquan

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/
-----------------------------------------------------------

(Updated 一月 12, 2015, 9:10 a.m.)


Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.


Repository: sentry


Description
-------

Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
1. Can't satisfied with the needs of dynamically add, delete and update permissions
2. Can't be centrally managed and difficult to maintain
According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).


Diffs (updated)
-----

  sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
  sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
  sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
  sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
  sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
  sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
  sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 

Diff: https://reviews.apache.org/r/29238/diff/


Testing
-------


Thanks,

shen guoquan

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.


> On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote:
> > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java, line 25
> > <https://reviews.apache.org/r/29238/diff/2/?file=799864#file799864line25>
> >
> >     what's "it"?  Where is the value set?  Also, I don't think we should have solr specific terminology in a search package, it should generally apply to search.

-->what's "it"
  The property of sentry.search.cluster was used to distinct itself from multiple search clusters. For example, there are two
  search clusters: cluster1 and cluster2 implemented authorization via sentry, and it must set the value of
  sentry.search.cluster=cluster1 or cluster2 to communicate with sentry service for authorization
  
-->I don't think we should have solr specific terminology in a search package, it should generally apply to search
  Thanks for your comment. I totally agree with you. I will fix it.


> On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote:
> > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java, line 29
> > <https://reviews.apache.org/r/29238/diff/2/?file=799864#file799864line29>
> >
> >     I saw your comment in the previous review.  I don't think the word "server" makes much sense in this context.  I think this is borrowing from hive terminology, where a server is actually distinct, but in solr, multiple "servers" may want to talk to the same sentry service and use the same policy, because they are logically part of the same (SolrCloud) service.  Maybe rename to service or cluster?  Since I don't think solr should be in the name either, maybe "sentry.search.service"?  Or "sentry.search.cluster"?

I think the "sentry.search.cluster" is more suitable. Thanks for your advice.


> On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 99
> > <https://reviews.apache.org/r/29238/diff/2/?file=799869#file799869line99>
> >
> >     Why is this SOLR specific in the search package?

The solr integration depends on the sentry new feature "generic model"(SENTRY-398). The privilege stored in the generic model database has a component name column, so which component wanting to integrate into sentry must have a component identity. For example, when the solr want to integrate into sentry, it must transfer solr name to sentry service.


> On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 120
> > <https://reviews.apache.org/r/29238/diff/2/?file=799869#file799869line120>
> >
> >     In the previous review, you explained the parameter "requestor" in the ProviderBackend as being filled in on the service side by the SolrHadoopAuthenticationFilter.  That makes sense.
> >     
> >     But this is on the client side, right?  Why is the requestor present on the client side?

-->Why is the requestor present on the client side
The solr service uses the SolrHadoopAuthenticationFilter to authenticate and gets the username. According to sentry service, the solr service is the client side. So when solr service communicates with sentry service, it must set the requestor.
but I agree with your concern about why the requestor has been transferred with a parameter, why the sentry service can't get the requestor using the SASL framework when the connection has set up. Currently the design part of sentry about getting the authenticate user is improper. I have already investigated the authentication of thrift service, I will fire a jira to fix the problem. Thanks for your precious opinions


> On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 236
> > <https://reviews.apache.org/r/29238/diff/2/?file=799869#file799869line236>
> >
> >     Why isn't this called listRolesByGroupName?

Thanks for your comments. I will fix it.


> On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, line 188
> > <https://reviews.apache.org/r/29238/diff/2/?file=799869#file799869line188>
> >
> >     Spelling.

I am sorry about this simple mistake. Thanks for pointing out.


> On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java, line 49
> > <https://reviews.apache.org/r/29238/diff/2/?file=799870#file799870line49>
> >
> >     Maybe call it SearchProviderBackend here to match the new name?

I am sorry about this simple mistake. Thanks for pointing out.


> On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java, line 72
> > <https://reviews.apache.org/r/29238/diff/2/?file=799870#file799870line72>
> >
> >     here too.

I am sorry about this simple mistake. Thanks for pointing out.


- shen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review67357
-----------------------------------------------------------


On 十二月 24, 2014, 3:30 a.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated 十二月 24, 2014, 3:30 a.m.)
> 
> 
> Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
>   sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by Gregory Chanan <gc...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/#review67357
-----------------------------------------------------------



sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java
<https://reviews.apache.org/r/29238/#comment111360>

    what's "it"?  Where is the value set?  Also, I don't think we should have solr specific terminology in a search package, it should generally apply to search.



sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java
<https://reviews.apache.org/r/29238/#comment111359>

    I saw your comment in the previous review.  I don't think the word "server" makes much sense in this context.  I think this is borrowing from hive terminology, where a server is actually distinct, but in solr, multiple "servers" may want to talk to the same sentry service and use the same policy, because they are logically part of the same (SolrCloud) service.  Maybe rename to service or cluster?  Since I don't think solr should be in the name either, maybe "sentry.search.service"?  Or "sentry.search.cluster"?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment111361>

    Why is this SOLR specific in the search package?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment111370>

    In the previous review, you explained the parameter "requestor" in the ProviderBackend as being filled in on the service side by the SolrHadoopAuthenticationFilter.  That makes sense.
    
    But this is on the client side, right?  Why is the requestor present on the client side?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment111362>

    you delete a role from a group, not "to" a group.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment111363>

    Spelling.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java
<https://reviews.apache.org/r/29238/#comment111366>

    Why isn't this called listRolesByGroupName?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java
<https://reviews.apache.org/r/29238/#comment111367>

    Maybe call it SearchProviderBackend here to match the new name?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java
<https://reviews.apache.org/r/29238/#comment111368>

    here too.


- Gregory Chanan


On Dec. 24, 2014, 3:30 a.m., shen guoquan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29238/
> -----------------------------------------------------------
> 
> (Updated Dec. 24, 2014, 3:30 a.m.)
> 
> 
> Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
> 1. Can't satisfied with the needs of dynamically add, delete and update permissions
> 2. Can't be centrally managed and difficult to maintain
> According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
>   sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
>   sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
>   sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
>   sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
>   sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
>   sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 
> 
> Diff: https://reviews.apache.org/r/29238/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> shen guoquan
> 
>

Re: Review Request 29238: SENTRY-479:Create a Solr client to interactive with the sentry service

Posted by shen guoquan <gu...@intel.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29238/
-----------------------------------------------------------

(Updated 十二月 24, 2014, 3:30 a.m.)


Review request for sentry, Gregory Chanan and Vamsee Yarlagadda.


Repository: sentry


Description
-------

Currently Solr does support index-level security via sentry with file as backend privilege store. Using file as the backend store is simple, but there has some disadvantages as followings:
1. Can't satisfied with the needs of dynamically add, delete and update permissions
2. Can't be centrally managed and difficult to maintain
According to the above disadvantages, The Solr Sentry plug-in integration with DB store is demanded. The Hive Sentry plug-in has already integration with DB store, but the Hive authorization model is different from the Solr authorization model.So this new feature depends on the generic authorization model(SENTRY-398).


Diffs (updated)
-----

  sentry-binding/sentry-binding-solr/pom.xml 2dfc933 
  sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f 
  sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java 7a39b79 
  sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java f428aea 
  sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 4b98447 
  sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java ddb9cf9 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java ea8eb79 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java PRE-CREATION 
  sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java fa5ab69 

Diff: https://reviews.apache.org/r/29238/diff/


Testing
-------


Thanks,

shen guoquan