You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Lenni Kuff <ls...@cloudera.com> on 2015/02/19 07:09:47 UTC
Re: Review Request 29239: SENTRY-481:Add Solr e2e test for
integrating with DB store
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29239/#review73090
-----------------------------------------------------------
Ship it!
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java
<https://reviews.apache.org/r/29239/#comment119290>
consider naming these:
GROUP=admin
ROLE=admin_role
COLLECTION_NAME=admin_collection
it might make it easier to debug if there is a test failure (otherwise there are a lot of things named "admin")
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java
<https://reviews.apache.org/r/29239/#comment119292>
If a new property is set, it looks like it is important to unset it. Perhaps split this out in to two functions - setSystemProperties / unsetSystemProperties and add them next to eachother (line number wise) so it is very easy to see what is and what is not being set. Also comment that it is important to unset these props.
Question - why set these as system properties rather than in the conf object?
sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java
<https://reviews.apache.org/r/29239/#comment119291>
thanks for commenting the test cases, this makes it easy to see what's going on without reading all the code.
- Lenni Kuff
On Feb. 19, 2015, 5:33 a.m., shen guoquan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29239/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2015, 5:33 a.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, Lenni Kuff, 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:
>
> Can't satisfied with the needs of dynamically add, delete and update permissions
> 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-tests/sentry-tests-solr/pom.xml dfc3792
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java 6d2550b
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrAdminOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrDocLevelOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrQueryOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrUpdateOperations.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29239/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> shen guoquan
>
>
Re: Review Request 29239: SENTRY-481:Add Solr e2e test for
integrating with DB store
Posted by shen guoquan <gu...@intel.com>.
> On Feb. 19, 2015, 6:09 a.m., Lenni Kuff wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java, line 64
> > <https://reviews.apache.org/r/29239/diff/3/?file=839746#file839746line64>
> >
> > consider naming these:
> >
> > GROUP=admin
> > ROLE=admin_role
> > COLLECTION_NAME=admin_collection
> >
> > it might make it easier to debug if there is a test failure (otherwise there are a lot of things named "admin")
Thanks Lenni for your good advice. I will fix it.
> On Feb. 19, 2015, 6:09 a.m., Lenni Kuff wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java, line 98
> > <https://reviews.apache.org/r/29239/diff/3/?file=839746#file839746line98>
> >
> > If a new property is set, it looks like it is important to unset it. Perhaps split this out in to two functions - setSystemProperties / unsetSystemProperties and add them next to eachother (line number wise) so it is very easy to see what is and what is not being set. Also comment that it is important to unset these props.
> >
> > Question - why set these as system properties rather than in the conf object?
I will put these properties in two new functions. Thanks for your suggestion.
-->why set these as system properties rather than in the conf object?
Because the solr binding will get these from properties from the system properties not from the configuration.
> On Feb. 19, 2015, 6:09 a.m., Lenni Kuff wrote:
> > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java, line 256
> > <https://reviews.apache.org/r/29239/diff/3/?file=839746#file839746line256>
> >
> > thanks for commenting the test cases, this makes it easy to see what's going on without reading all the code.
Thanks Lenni. I will fix it
- shen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29239/#review73090
-----------------------------------------------------------
On Feb. 19, 2015, 5:33 a.m., shen guoquan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29239/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2015, 5:33 a.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, Lenni Kuff, 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:
>
> Can't satisfied with the needs of dynamically add, delete and update permissions
> 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-tests/sentry-tests-solr/pom.xml dfc3792
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/AbstractSolrSentryTestBase.java 6d2550b
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrAdminOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrDocLevelOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrQueryOperations.java PRE-CREATION
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrUpdateOperations.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29239/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> shen guoquan
>
>