You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Dapeng Sun <da...@intel.com> on 2015/01/28 14:55:04 UTC
Review Request 30363: SENTRY-296 Sentry Service Client does not allow
for connection pooling
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30363/
-----------------------------------------------------------
Review request for sentry and Lenni Kuff.
Bugs: SENTRY-296
https://issues.apache.org/jira/browse/SENTRY-296
Repository: sentry
Description
-------
Since the change in **SENTRY-600 Refactor sentry client factory for "high availability" and "connection pool"** is too complicated, I refactor this part, this review request also merge **SENTRY-601 Create connection pool factory**
I think this patch might minimized the code change
* Kept the method **public static SentryPolicyServiceClient create(Configuration conf) throws Exception**
* Add **Generic Type** support to HAClientInvocationHandler and PoolClientInvocationHandler, it will make integration with other client more easily
* Put **Connection pool instance** into PoolInvocationHandler, it will make Client connection can be pooled.
Diffs
-----
pom.xml 60a9f4a
sentry-provider/sentry-provider-db/pom.xml 6116cd5
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/service/thrift/HAClientInvocationHandler.java c6e265f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java PRE-CREATION
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/SentryServiceClientPoolFactory.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java ddc5930
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolHAWithoutKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolWithoutKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolHAWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java ca64ce1
Diff: https://reviews.apache.org/r/30363/diff/
Testing
-------
Unit Tests passed
Thanks,
Dapeng Sun
Re: Review Request 30363: SENTRY-296 Sentry Service Client does not
allow for connection pooling
Posted by Dapeng Sun <da...@intel.com>.
> On 四月 1, 2015, 10:23 a.m., Prasad Mujumdar wrote:
> > Also we need similar change for the SentryHDFS client. Please log a followup ticket to track that.
Thank you for pointing out it. I will create a jira to track it.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30363/#review78468
-----------------------------------------------------------
On 二月 10, 2015, 9:33 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30363/
> -----------------------------------------------------------
>
> (Updated 二月 10, 2015, 9:33 a.m.)
>
>
> Review request for sentry and Lenni Kuff.
>
>
> Bugs: SENTRY-296
> https://issues.apache.org/jira/browse/SENTRY-296
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since the change in **SENTRY-600 Refactor sentry client factory for "high availability" and "connection pool"** is too complicated, I refactor this part, this review request also merge **SENTRY-601 Create connection pool factory**
>
> I think this patch might minimized the code change
> * Kept the method **public static SentryPolicyServiceClient create(Configuration conf) throws Exception**
> * Add **Generic Type** support to HAClientInvocationHandler and PoolClientInvocationHandler, it will make integration with other client more easily
> * Put **Connection pool instance** into PoolInvocationHandler, it will make Client connection can be pooled.
>
>
> Diffs
> -----
>
> pom.xml 60a9f4a
> sentry-provider/sentry-provider-db/pom.xml 6116cd5
> 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/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java PRE-CREATION
> 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/SentryServiceClientPoolFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java ddc5930
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolHAWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolHAWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java ca64ce1
>
> Diff: https://reviews.apache.org/r/30363/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 30363: SENTRY-296 Sentry Service Client does not
allow for connection pooling
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30363/#review78468
-----------------------------------------------------------
Also we need similar change for the SentryHDFS client. Please log a followup ticket to track that.
- Prasad Mujumdar
On Feb. 10, 2015, 1:33 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30363/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2015, 1:33 a.m.)
>
>
> Review request for sentry and Lenni Kuff.
>
>
> Bugs: SENTRY-296
> https://issues.apache.org/jira/browse/SENTRY-296
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since the change in **SENTRY-600 Refactor sentry client factory for "high availability" and "connection pool"** is too complicated, I refactor this part, this review request also merge **SENTRY-601 Create connection pool factory**
>
> I think this patch might minimized the code change
> * Kept the method **public static SentryPolicyServiceClient create(Configuration conf) throws Exception**
> * Add **Generic Type** support to HAClientInvocationHandler and PoolClientInvocationHandler, it will make integration with other client more easily
> * Put **Connection pool instance** into PoolInvocationHandler, it will make Client connection can be pooled.
>
>
> Diffs
> -----
>
> pom.xml 60a9f4a
> sentry-provider/sentry-provider-db/pom.xml 6116cd5
> 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/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java PRE-CREATION
> 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/SentryServiceClientPoolFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java ddc5930
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolHAWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolHAWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java ca64ce1
>
> Diff: https://reviews.apache.org/r/30363/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 30363: SENTRY-296 Sentry Service Client does not
allow for connection pooling
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30363/#review80919
-----------------------------------------------------------
Ship it!
Please log the tickets for follow item discussed in the review.
- Prasad Mujumdar
On April 1, 2015, 12:05 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30363/
> -----------------------------------------------------------
>
> (Updated April 1, 2015, 12:05 p.m.)
>
>
> Review request for sentry and Lenni Kuff.
>
>
> Bugs: SENTRY-296
> https://issues.apache.org/jira/browse/SENTRY-296
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since the change in **SENTRY-600 Refactor sentry client factory for "high availability" and "connection pool"** is too complicated, I refactor this part, this review request also merge **SENTRY-601 Create connection pool factory**
>
> I think this patch might minimized the code change
> * Kept the method **public static SentryPolicyServiceClient create(Configuration conf) throws Exception**
> * Add **Generic Type** support to HAClientInvocationHandler and PoolClientInvocationHandler, it will make integration with other client more easily
> * Put **Connection pool instance** into PoolInvocationHandler, it will make Client connection can be pooled.
>
>
> Diffs
> -----
>
> pom.xml cd594b5
> sentry-provider/sentry-provider-db/pom.xml 27ad670
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java 4947ad1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java PRE-CREATION
> 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/SentryServiceClientPoolFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java c8f7450
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolHAWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolHAWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 9a6f8c4
>
> Diff: https://reviews.apache.org/r/30363/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 30363: SENTRY-296 Sentry Service Client does not
allow for connection pooling
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30363/
-----------------------------------------------------------
(Updated 四月 1, 2015, 8:05 p.m.)
Review request for sentry and Lenni Kuff.
Changes
-------
Updated patch according Prasad's comments
Bugs: SENTRY-296
https://issues.apache.org/jira/browse/SENTRY-296
Repository: sentry
Description
-------
Since the change in **SENTRY-600 Refactor sentry client factory for "high availability" and "connection pool"** is too complicated, I refactor this part, this review request also merge **SENTRY-601 Create connection pool factory**
I think this patch might minimized the code change
* Kept the method **public static SentryPolicyServiceClient create(Configuration conf) throws Exception**
* Add **Generic Type** support to HAClientInvocationHandler and PoolClientInvocationHandler, it will make integration with other client more easily
* Put **Connection pool instance** into PoolInvocationHandler, it will make Client connection can be pooled.
Diffs (updated)
-----
pom.xml cd594b5
sentry-provider/sentry-provider-db/pom.xml 27ad670
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java 4947ad1
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java PRE-CREATION
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/SentryServiceClientPoolFactory.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java c8f7450
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolHAWithoutKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolWithoutKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolHAWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 9a6f8c4
Diff: https://reviews.apache.org/r/30363/diff/
Testing
-------
Unit Tests passed
Thanks,
Dapeng Sun
Re: Review Request 30363: SENTRY-296 Sentry Service Client does not
allow for connection pooling
Posted by Dapeng Sun <da...@intel.com>.
> On 四月 1, 2015, 10:23 a.m., Prasad Mujumdar wrote:
> > At high level, the implementation seems to be creating a pool per client. For example, each Hive session will have it's own pool. Is that how we intended the pooling to work ? or should the pool factory be static and all hive or other Sentry clients sessions can share the pool ?
> >
> > The patch itself look fine. A few comments below.
Thank Prasad for your review.
Yes, the implementation is creating a pool per client. I also considered the way to share the pool for multi-client, but currently the method will be like **SentryServiceClientFactory.getClient(conf)**, it will be no problem if the **conf**s are same. if the **conf**s are different, it will be hard to create the static pool. another way is creating a factory instance, we can put the pool to the factory, this way will be like previous way[SENTRY-600.002.patch|https://issues.apache.org/jira/secure/attachment/12689104/SENTRY-600.002.patch], it need a lot of code change, people may not buy in it. On the other hand, putting the pool to client may be more flexible. How do you think about it?
> On 四月 1, 2015, 10:23 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java, line 122
> > <https://reviews.apache.org/r/30363/diff/3/?file=859356#file859356line122>
> >
> > Are we sure that TargetException can always be converted to SentryUserException ? Would it be better to wrapp it in SentryUserException instead ?
Thank you for your comment, I'm agreed with you. I will update it in next patch.
> On 四月 1, 2015, 10:23 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java, line 134
> > <https://reviews.apache.org/r/30363/diff/3/?file=859356#file859356line134>
> >
> > In case of exception in the above try block, the original exception will be lost. Would it be better to throw the original exception and not the one from returnObject() ?
Thank you for your comment, I'm agreed with you. I will update it in next patch.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30363/#review78464
-----------------------------------------------------------
On 二月 10, 2015, 9:33 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30363/
> -----------------------------------------------------------
>
> (Updated 二月 10, 2015, 9:33 a.m.)
>
>
> Review request for sentry and Lenni Kuff.
>
>
> Bugs: SENTRY-296
> https://issues.apache.org/jira/browse/SENTRY-296
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since the change in **SENTRY-600 Refactor sentry client factory for "high availability" and "connection pool"** is too complicated, I refactor this part, this review request also merge **SENTRY-601 Create connection pool factory**
>
> I think this patch might minimized the code change
> * Kept the method **public static SentryPolicyServiceClient create(Configuration conf) throws Exception**
> * Add **Generic Type** support to HAClientInvocationHandler and PoolClientInvocationHandler, it will make integration with other client more easily
> * Put **Connection pool instance** into PoolInvocationHandler, it will make Client connection can be pooled.
>
>
> Diffs
> -----
>
> pom.xml 60a9f4a
> sentry-provider/sentry-provider-db/pom.xml 6116cd5
> 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/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java PRE-CREATION
> 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/SentryServiceClientPoolFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java ddc5930
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolHAWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolHAWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java ca64ce1
>
> Diff: https://reviews.apache.org/r/30363/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 30363: SENTRY-296 Sentry Service Client does not
allow for connection pooling
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On April 1, 2015, 2:23 a.m., Prasad Mujumdar wrote:
> > At high level, the implementation seems to be creating a pool per client. For example, each Hive session will have it's own pool. Is that how we intended the pooling to work ? or should the pool factory be static and all hive or other Sentry clients sessions can share the pool ?
> >
> > The patch itself look fine. A few comments below.
>
> Dapeng Sun wrote:
> Thank Prasad for your review.
> Yes, the implementation is creating a pool per client. I also considered the way to share the pool for multi-client, but currently the method will be like **SentryServiceClientFactory.getClient(conf)**, it will be no problem if the **conf**s are same. if the **conf**s are different, it will be hard to create the static pool. another way is creating a factory instance, we can put the pool to the factory, this way will be like previous way[SENTRY-600.002.patch|https://issues.apache.org/jira/secure/attachment/12689104/SENTRY-600.002.patch], it need a lot of code change, people may not buy in it. On the other hand, putting the pool to client may be more flexible. How do you think about it?
ok.
I guess it will more useful for clients like Impala that keeps a single connection to cache policies. I think it should be fine to restrict per instance configuration (at least for service related properties) when pooling is enabled.
Let's log a followup ticket to support that. I think a shared connection pool for Hive and similar service clients would be very useful for optimizing the connection management.
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30363/#review78464
-----------------------------------------------------------
On April 1, 2015, 12:05 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30363/
> -----------------------------------------------------------
>
> (Updated April 1, 2015, 12:05 p.m.)
>
>
> Review request for sentry and Lenni Kuff.
>
>
> Bugs: SENTRY-296
> https://issues.apache.org/jira/browse/SENTRY-296
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since the change in **SENTRY-600 Refactor sentry client factory for "high availability" and "connection pool"** is too complicated, I refactor this part, this review request also merge **SENTRY-601 Create connection pool factory**
>
> I think this patch might minimized the code change
> * Kept the method **public static SentryPolicyServiceClient create(Configuration conf) throws Exception**
> * Add **Generic Type** support to HAClientInvocationHandler and PoolClientInvocationHandler, it will make integration with other client more easily
> * Put **Connection pool instance** into PoolInvocationHandler, it will make Client connection can be pooled.
>
>
> Diffs
> -----
>
> pom.xml cd594b5
> sentry-provider/sentry-provider-db/pom.xml 27ad670
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java 4947ad1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java PRE-CREATION
> 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/SentryServiceClientPoolFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java c8f7450
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolHAWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolHAWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 9a6f8c4
>
> Diff: https://reviews.apache.org/r/30363/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 30363: SENTRY-296 Sentry Service Client does not
allow for connection pooling
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30363/#review78464
-----------------------------------------------------------
At high level, the implementation seems to be creating a pool per client. For example, each Hive session will have it's own pool. Is that how we intended the pooling to work ? or should the pool factory be static and all hive or other Sentry clients sessions can share the pool ?
The patch itself look fine. A few comments below.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
<https://reviews.apache.org/r/30363/#comment127214>
Are we sure that TargetException can always be converted to SentryUserException ? Would it be better to wrapp it in SentryUserException instead ?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java
<https://reviews.apache.org/r/30363/#comment127215>
In case of exception in the above try block, the original exception will be lost. Would it be better to throw the original exception and not the one from returnObject() ?
- Prasad Mujumdar
On Feb. 10, 2015, 1:33 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30363/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2015, 1:33 a.m.)
>
>
> Review request for sentry and Lenni Kuff.
>
>
> Bugs: SENTRY-296
> https://issues.apache.org/jira/browse/SENTRY-296
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since the change in **SENTRY-600 Refactor sentry client factory for "high availability" and "connection pool"** is too complicated, I refactor this part, this review request also merge **SENTRY-601 Create connection pool factory**
>
> I think this patch might minimized the code change
> * Kept the method **public static SentryPolicyServiceClient create(Configuration conf) throws Exception**
> * Add **Generic Type** support to HAClientInvocationHandler and PoolClientInvocationHandler, it will make integration with other client more easily
> * Put **Connection pool instance** into PoolInvocationHandler, it will make Client connection can be pooled.
>
>
> Diffs
> -----
>
> pom.xml 60a9f4a
> sentry-provider/sentry-provider-db/pom.xml 6116cd5
> 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/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java PRE-CREATION
> 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/SentryServiceClientPoolFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java ddc5930
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolHAWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolHAWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java ca64ce1
>
> Diff: https://reviews.apache.org/r/30363/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 30363: SENTRY-296 Sentry Service Client does not
allow for connection pooling
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30363/
-----------------------------------------------------------
(Updated 二月 10, 2015, 9:33 a.m.)
Review request for sentry and Lenni Kuff.
Changes
-------
Update patch minimized the code change
Bugs: SENTRY-296
https://issues.apache.org/jira/browse/SENTRY-296
Repository: sentry
Description
-------
Since the change in **SENTRY-600 Refactor sentry client factory for "high availability" and "connection pool"** is too complicated, I refactor this part, this review request also merge **SENTRY-601 Create connection pool factory**
I think this patch might minimized the code change
* Kept the method **public static SentryPolicyServiceClient create(Configuration conf) throws Exception**
* Add **Generic Type** support to HAClientInvocationHandler and PoolClientInvocationHandler, it will make integration with other client more easily
* Put **Connection pool instance** into PoolInvocationHandler, it will make Client connection can be pooled.
Diffs (updated)
-----
pom.xml 60a9f4a
sentry-provider/sentry-provider-db/pom.xml 6116cd5
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/PoolClientInvocationHandler.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java PRE-CREATION
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/SentryServiceClientPoolFactory.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java ddc5930
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolHAWithoutKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolWithoutKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolHAWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java ca64ce1
Diff: https://reviews.apache.org/r/30363/diff/
Testing
-------
Unit Tests passed
Thanks,
Dapeng Sun
Re: Review Request 30363: SENTRY-296 Sentry Service Client does not
allow for connection pooling
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30363/
-----------------------------------------------------------
(Updated 二月 3, 2015, 9:12 p.m.)
Review request for sentry and Lenni Kuff.
Changes
-------
Updated the patch accroding Lenni's feedback.
Bugs: SENTRY-296
https://issues.apache.org/jira/browse/SENTRY-296
Repository: sentry
Description
-------
Since the change in **SENTRY-600 Refactor sentry client factory for "high availability" and "connection pool"** is too complicated, I refactor this part, this review request also merge **SENTRY-601 Create connection pool factory**
I think this patch might minimized the code change
* Kept the method **public static SentryPolicyServiceClient create(Configuration conf) throws Exception**
* Add **Generic Type** support to HAClientInvocationHandler and PoolClientInvocationHandler, it will make integration with other client more easily
* Put **Connection pool instance** into PoolInvocationHandler, it will make Client connection can be pooled.
Diffs (updated)
-----
pom.xml 60a9f4a
sentry-provider/sentry-provider-db/pom.xml 6116cd5
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/service/thrift/HAClientInvocationHandler.java c6e265f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java PRE-CREATION
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/SentryServiceClientPoolFactory.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java ddc5930
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolHAWithoutKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolWithoutKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolHAWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java ca64ce1
Diff: https://reviews.apache.org/r/30363/diff/
Testing
-------
Unit Tests passed
Thanks,
Dapeng Sun
Re: Review Request 30363: SENTRY-296 Sentry Service Client does not
allow for connection pooling
Posted by Dapeng Sun <da...@intel.com>.
> On 二月 2, 2015, 4:02 p.m., Lenni Kuff wrote:
> > I think this looks much better, thanks. A few comments below. The use of reflection/Proxy classes makes the code a bit harder to follow, so if you could add a few comments on that it might be helpeful. I understand why it's useful here though.
Thank you for your suggestion, I will add the comments in next patch.
> On 二月 2, 2015, 4:02 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java, line 100
> > <https://reviews.apache.org/r/30363/diff/1/?file=838784#file838784line100>
> >
> > spelling avaible
Thank you for your suggestion, I will fix it in next patch.
> On 二月 2, 2015, 4:02 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java, line 31
> > <https://reviews.apache.org/r/30363/diff/1/?file=838786#file838786line31>
> >
> > why would you want to call a method that's not accessible?
Thank you for your suggestion, I will fix it in next patch.
> On 二月 2, 2015, 4:02 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceBaseClient.java, line 23
> > <https://reviews.apache.org/r/30363/diff/1/?file=838782#file838782line23>
> >
> > Why do we need this interface? All it does is add the "close()" method. Can you just add "close()" to the SentryPolicyServiceClient interface?
Hi Lenni, currently we have two kinds of client. one is **SentryPolicyServiceClient**, another is **SentryGenericServiceClient**, it's better to have a common interface for client, so that we can handle the HA or pool in a single InvovationHandler.If you think the change should not be added at here, I will remove it.
> On 二月 2, 2015, 4:02 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java, line 107
> > <https://reviews.apache.org/r/30363/diff/1/?file=838784#file838784line107>
> >
> > why are you setting HA_ENABLED = false?
**ClientFactory.create()** have some hierarchies, likes **DefaultClient->HAProxy->PooledProxy**, **DefaultClient->PooledProxy** or **DefaultClient->HAProxy**. When the code proceeds to here, HA flag in the configuration must be true, if we don't set to false here, it will cause a problem of endless loop.
> On 二月 2, 2015, 4:02 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java, line 33
> > <https://reviews.apache.org/r/30363/diff/1/?file=838786#file838786line33>
> >
> > Should you add a Preconditions check here to verify that the "close" method doesn't is a version that takes no params?
Thank you for your suggestion, I will fix it in next patch.
> On 二月 2, 2015, 4:02 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java, line 47
> > <https://reviews.apache.org/r/30363/diff/1/?file=838788#file838788line47>
> >
> > Why are you setting this to false here?
**ClientFactory.create()** have some hierarchies, likes **DefaultClient->HAProxy->PooledProxy**, **DefaultClient->PooledProxy** or **DefaultClient->HAProxy**. When the code proceeds to here, HA flag in the configuration must be true, if we don't set to false here, it will cause a problem of endless loop.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30363/#review70543
-----------------------------------------------------------
On 一月 28, 2015, 9:55 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30363/
> -----------------------------------------------------------
>
> (Updated 一月 28, 2015, 9:55 p.m.)
>
>
> Review request for sentry and Lenni Kuff.
>
>
> Bugs: SENTRY-296
> https://issues.apache.org/jira/browse/SENTRY-296
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since the change in **SENTRY-600 Refactor sentry client factory for "high availability" and "connection pool"** is too complicated, I refactor this part, this review request also merge **SENTRY-601 Create connection pool factory**
>
> I think this patch might minimized the code change
> * Kept the method **public static SentryPolicyServiceClient create(Configuration conf) throws Exception**
> * Add **Generic Type** support to HAClientInvocationHandler and PoolClientInvocationHandler, it will make integration with other client more easily
> * Put **Connection pool instance** into PoolInvocationHandler, it will make Client connection can be pooled.
>
>
> Diffs
> -----
>
> pom.xml 60a9f4a
> sentry-provider/sentry-provider-db/pom.xml 6116cd5
> 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/service/thrift/HAClientInvocationHandler.java c6e265f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java PRE-CREATION
> 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/SentryServiceClientPoolFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java ddc5930
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolHAWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolHAWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java ca64ce1
>
> Diff: https://reviews.apache.org/r/30363/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 30363: SENTRY-296 Sentry Service Client does not
allow for connection pooling
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30363/#review70543
-----------------------------------------------------------
I think this looks much better, thanks. A few comments below. The use of reflection/Proxy classes makes the code a bit harder to follow, so if you could add a few comments on that it might be helpeful. I understand why it's useful here though.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceBaseClient.java
<https://reviews.apache.org/r/30363/#comment115760>
Why do we need this interface? All it does is add the "close()" method. Can you just add "close()" to the SentryPolicyServiceClient interface?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java
<https://reviews.apache.org/r/30363/#comment115762>
spelling avaible
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java
<https://reviews.apache.org/r/30363/#comment115763>
why are you setting HA_ENABLED = false?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
<https://reviews.apache.org/r/30363/#comment115768>
why would you want to call a method that's not accessible?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java
<https://reviews.apache.org/r/30363/#comment115761>
Should you add a Preconditions check here to verify that the "close" method doesn't is a version that takes no params?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java
<https://reviews.apache.org/r/30363/#comment115767>
Why are you setting this to false here?
- Lenni Kuff
On Jan. 28, 2015, 1:55 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30363/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2015, 1:55 p.m.)
>
>
> Review request for sentry and Lenni Kuff.
>
>
> Bugs: SENTRY-296
> https://issues.apache.org/jira/browse/SENTRY-296
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Since the change in **SENTRY-600 Refactor sentry client factory for "high availability" and "connection pool"** is too complicated, I refactor this part, this review request also merge **SENTRY-601 Create connection pool factory**
>
> I think this patch might minimized the code change
> * Kept the method **public static SentryPolicyServiceClient create(Configuration conf) throws Exception**
> * Add **Generic Type** support to HAClientInvocationHandler and PoolClientInvocationHandler, it will make integration with other client more easily
> * Put **Connection pool instance** into PoolInvocationHandler, it will make Client connection can be pooled.
>
>
> Diffs
> -----
>
> pom.xml 60a9f4a
> sentry-provider/sentry-provider-db/pom.xml 6116cd5
> 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/service/thrift/HAClientInvocationHandler.java c6e265f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java PRE-CREATION
> 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/SentryServiceClientPoolFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java ddc5930
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolHAWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForPoolWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolHAWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForPoolWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java ca64ce1
>
> Diff: https://reviews.apache.org/r/30363/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>