You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Li Li <li...@cloudera.com> on 2015/11/09 20:28:31 UTC
Re: Review Request 39801: SENTRY-498: Sentry integration with Hive
authorization framework V2
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39801/#review105703
-----------------------------------------------------------
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingSessionHookV2.java (line 31)
<https://reviews.apache.org/r/39801/#comment164335>
Though you've mentioned in the patch comment, it will be better if we can add some comments for these new added classes.
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingSessionHookV2.java (line 37)
<https://reviews.apache.org/r/39801/#comment164307>
add 2 more spaces before each line from line 37 to 54?
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingSessionHookV2.java (line 64)
<https://reviews.apache.org/r/39801/#comment164317>
sensetive -> sensitive ?
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingSessionHookV2.java (line 121)
<https://reviews.apache.org/r/39801/#comment164330>
seems the implementation of loadAuthzConf is same as that of loadAuthzConf in HiveAuthzBindingHook.java and also SentryAuthorizerFactory.java. I wonder if we can avoid the duplicate code.
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java (line 381)
<https://reviews.apache.org/r/39801/#comment164347>
Maybe we can just remove this line?
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SentryAuthorizerUtil.java (line 267)
<https://reviews.apache.org/r/39801/#comment164346>
should we log this for case default?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java (line 38)
<https://reviews.apache.org/r/39801/#comment164342>
seems we have not used junit.Ignore in this class?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java (line 61)
<https://reviews.apache.org/r/39801/#comment164337>
I wonder why we ignore this test?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java (line 157)
<https://reviews.apache.org/r/39801/#comment164338>
HADOOP_BIN -> HADOOPBIN?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java (line 241)
<https://reviews.apache.org/r/39801/#comment164340>
should we log this for case default?
- Li Li
On Oct. 30, 2015, 3:45 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39801/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2015, 3:45 a.m.)
>
>
> Review request for sentry, Colin Ma, shen guoquan, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-498
> https://issues.apache.org/jira/browse/SENTRY-498
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch is based on Hive authorization V2 framework.
> Through this framework, we will change more less code to integrate with Hive version in future.
> We add the new module hive-binding-v2, the previous binding will not change, it will still work.
> Currently the patch change the base class of Hive E2E tests, I do this just to show what changes need to add E2E test, we should separate a E2E module for binding V2 in future.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHook.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingSessionHookV2.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/SentryAuthorizerFactory.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/SentryHiveAuthorizationTaskFactoryImplV2.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryAccessController.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/SentryHiveAuthorizer.java 4aa6948
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/AuthorizingObjectStoreV2.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/MetastoreAuthzBindingV2.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/SentryMetastorePostEventListenerV2.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SentryAuthorizerUtil.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SimpleSemanticAnalyzer.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/test/java/org/apache/sentry/binding/hive/v2/DummyHiveAuthenticationProvider.java PRE-CREATION
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java d9bb42d
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStore.java 5a0c950
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java 5375f6a
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 2ae4fbd
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java 4924669
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Column.java 89aabfc
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java fbb611e
> sentry-tests/sentry-tests-hive/pom.xml 0a5b791
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 17a2d1e
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 718a736
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java 98de57d
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java e61dff0
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc5daef
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestMetadataObjectRetrieval.java 71e3af2
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java 7ca3b67
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestServerConfiguration.java 18fc5d9
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 1014361
>
> Diff: https://reviews.apache.org/r/39801/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 39801: SENTRY-498: Sentry integration with Hive
authorization framework V2
Posted by Dapeng Sun <da...@intel.com>.
> On 十一月 10, 2015, 3:28 a.m., Li Li wrote:
> >
Thank you Li, Li for your review.
> On 十一月 10, 2015, 3:28 a.m., Li Li wrote:
> > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingSessionHookV2.java, line 31
> > <https://reviews.apache.org/r/39801/diff/1/?file=1112792#file1112792line31>
> >
> > Though you've mentioned in the patch comment, it will be better if we can add some comments for these new added classes.
Good suggestion, I will update the patch.
> On 十一月 10, 2015, 3:28 a.m., Li Li wrote:
> > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingSessionHookV2.java, line 37
> > <https://reviews.apache.org/r/39801/diff/1/?file=1112792#file1112792line37>
> >
> > add 2 more spaces before each line from line 37 to 54?
Good suggestion.
> On 十一月 10, 2015, 3:28 a.m., Li Li wrote:
> > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingSessionHookV2.java, line 64
> > <https://reviews.apache.org/r/39801/diff/1/?file=1112792#file1112792line64>
> >
> > sensetive -> sensitive ?
Good suggestion.
> On 十一月 10, 2015, 3:28 a.m., Li Li wrote:
> > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingSessionHookV2.java, line 121
> > <https://reviews.apache.org/r/39801/diff/1/?file=1112792#file1112792line121>
> >
> > seems the implementation of loadAuthzConf is same as that of loadAuthzConf in HiveAuthzBindingHook.java and also SentryAuthorizerFactory.java. I wonder if we can avoid the duplicate code.
Good suggestion.
> On 十一月 10, 2015, 3:28 a.m., Li Li wrote:
> > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java, line 381
> > <https://reviews.apache.org/r/39801/diff/1/?file=1112796#file1112796line381>
> >
> > Maybe we can just remove this line?
I will add a log to track the exception.
> On 十一月 10, 2015, 3:28 a.m., Li Li wrote:
> > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SentryAuthorizerUtil.java, line 267
> > <https://reviews.apache.org/r/39801/diff/1/?file=1112801#file1112801line267>
> >
> > should we log this for case default?
Good suggestion.
> On 十一月 10, 2015, 3:28 a.m., Li Li wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java, line 158
> > <https://reviews.apache.org/r/39801/diff/1/?file=1112820#file1112820line158>
> >
> > HADOOP_BIN -> HADOOPBIN?
I think HADOOPBIN is used before this patch, I think it is better to open a new ticket to fix it.
> On 十一月 10, 2015, 3:28 a.m., Li Li wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java, line 244
> > <https://reviews.apache.org/r/39801/diff/1/?file=1112820#file1112820line244>
> >
> > should we log this for case default?
Good suggestion.
> On 十一月 10, 2015, 3:28 a.m., Li Li wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java, line 61
> > <https://reviews.apache.org/r/39801/diff/1/?file=1112818#file1112818line61>
> >
> > I wonder why we ignore this test?
I think this is a issue of Unit Tests classpath isolation. I will do more research on it.
> On 十一月 10, 2015, 3:28 a.m., Li Li wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java, line 38
> > <https://reviews.apache.org/r/39801/diff/1/?file=1112813#file1112813line38>
> >
> > seems we have not used junit.Ignore in this class?
I will remove it.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39801/#review105703
-----------------------------------------------------------
On 十月 30, 2015, 11:45 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39801/
> -----------------------------------------------------------
>
> (Updated 十月 30, 2015, 11:45 a.m.)
>
>
> Review request for sentry, Colin Ma, shen guoquan, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-498
> https://issues.apache.org/jira/browse/SENTRY-498
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch is based on Hive authorization V2 framework.
> Through this framework, we will change more less code to integrate with Hive version in future.
> We add the new module hive-binding-v2, the previous binding will not change, it will still work.
> Currently the patch change the base class of Hive E2E tests, I do this just to show what changes need to add E2E test, we should separate a E2E module for binding V2 in future.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHook.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingSessionHookV2.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/SentryAuthorizerFactory.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/SentryHiveAuthorizationTaskFactoryImplV2.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryAccessController.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/SentryHiveAuthorizer.java 4aa6948
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/AuthorizingObjectStoreV2.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/MetastoreAuthzBindingV2.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/SentryMetastorePostEventListenerV2.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SentryAuthorizerUtil.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SimpleSemanticAnalyzer.java PRE-CREATION
> sentry-binding/sentry-binding-hive-v2/src/test/java/org/apache/sentry/binding/hive/v2/DummyHiveAuthenticationProvider.java PRE-CREATION
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java d9bb42d
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStore.java 5a0c950
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java 5375f6a
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 2ae4fbd
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java 4924669
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Column.java 89aabfc
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java fbb611e
> sentry-tests/sentry-tests-hive/pom.xml 0a5b791
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 17a2d1e
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 718a736
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java 98de57d
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java e61dff0
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc5daef
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestMetadataObjectRetrieval.java 71e3af2
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtFunctionScope.java 7ca3b67
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestServerConfiguration.java 18fc5d9
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 1014361
>
> Diff: https://reviews.apache.org/r/39801/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dapeng Sun
>
>