----------------------------------------------------------- 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 > >
