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/01/13 02:30:52 UTC
Re: Review Request 29418: SENTRY-600 Refactor sentry client factory
for "high availability" and "connection pool"
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29418/#review67774
-----------------------------------------------------------
Please be sure to comment on all public interfaces (and iterface methods). It's also usually good to add a class comment to all public classes.
Question - Why have a seperate Factory implementations for each client types? This seems to defeat the purpose of having a factory in the first place. What about having a single "Factory" class which returns the proper client type based on the configuration settings?
- Lenni Kuff
On Dec. 25, 2014, 10:48 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29418/
> -----------------------------------------------------------
>
> (Updated Dec. 25, 2014, 10:48 a.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-600
> https://issues.apache.org/jira/browse/SENTRY-600
>
>
> Repository: sentry
>
>
> Description
> -------
>
> In order to increase the extensibility of ClientFactory, more client support and more implementation of factory, use dynamic proxy to build the client factory.
> **SentryPolicyServiceBaseClient** and **SentryServiceClientFactory** is the common Interface for client and factory.
> **SentryServiceClientProxyFactory** will handle the integration of different factorys.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java 69e97a6
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java ecbd664
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java cc433d4
> 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/service/persistent/HAContext.java 523261e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceBaseClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 962228f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java d21a401
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java c6e265f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 574f23c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServicePolicyClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/SentryClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/SentryServiceClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/SentryServiceClientProxyFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/ha/HAClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/ha/HASentryServiceClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java be14afd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 04f50ed
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java f8cc1d0
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 7ebc0e4
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java 9d15c95
>
> Diff: https://reviews.apache.org/r/29418/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 29418: SENTRY-600 Refactor sentry client factory
for "high availability" and "connection pool"
Posted by Dapeng Sun <da...@intel.com>.
> On 一月 13, 2015, 9:30 a.m., Lenni Kuff wrote:
> > Please be sure to comment on all public interfaces (and iterface methods). It's also usually good to add a class comment to all public classes.
> >
> >
> > Question - Why have a seperate Factory implementations for each client types? This seems to defeat the purpose of having a factory in the first place. What about having a single "Factory" class which returns the proper client type based on the configuration settings?
Please be sure to comment on all public interfaces (and iterface methods). It's also usually good to add a class comment to all public classes.
**Good suggestion, I will update it in next patch.**
Question - Why have a seperate Factory implementations for each client types?
**It is a decoupling for different client, first, we may have different default clients, like SentryDefaultClient for HIVE and GeneralModelClient; sencond, we may use different combinations of Pooling, HA and Default, for example, we can enable Pooling or HA independently, or using both of them. **
**Seperating Factory implementations may be better to extend a new feature on client and add new combinations.**
This seems to defeat the purpose of having a factory in the first place. What about having a single "Factory" class which returns the proper client type based on the configuration settings?
**Hi Lenni, good question!
Currently, SentryClientFactory may using like these:
SentryClientFacorty.getClient(Conf), it’s okay for HA and default thrift client because calling the method will return a new SENTRY Client every time.**
**when we enable Connection Pool for SENTRY. We should add a “Connection Pool Instance” to store the clients, there may be two solution:
in current patch, I create a factory instance and put the “Connection Pool Instance” to the factory instance;
another one is using “Connection Pool Instance” as static, but it need a premise, conf should not be changed.**
**Do you think it is possiable to use the “Connection Pool Instance” as static? Any other suggestions are also welcome, thank you**
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29418/#review67774
-----------------------------------------------------------
On 十二月 25, 2014, 6:48 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29418/
> -----------------------------------------------------------
>
> (Updated 十二月 25, 2014, 6:48 p.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-600
> https://issues.apache.org/jira/browse/SENTRY-600
>
>
> Repository: sentry
>
>
> Description
> -------
>
> In order to increase the extensibility of ClientFactory, more client support and more implementation of factory, use dynamic proxy to build the client factory.
> **SentryPolicyServiceBaseClient** and **SentryServiceClientFactory** is the common Interface for client and factory.
> **SentryServiceClientProxyFactory** will handle the integration of different factorys.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java 69e97a6
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java ecbd664
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java cc433d4
> 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/service/persistent/HAContext.java 523261e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceBaseClient.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 962228f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java d21a401
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java c6e265f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java 574f23c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServicePolicyClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/SentryClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/SentryServiceClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/SentryServiceClientProxyFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/ha/HAClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/ha/HASentryServiceClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java be14afd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 04f50ed
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java f8cc1d0
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 7ebc0e4
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java 9d15c95
>
> Diff: https://reviews.apache.org/r/29418/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>