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/02 09:02:07 UTC
Re: 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/#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
>
>
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
>
>