You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Prasad Mujumdar <pr...@cloudera.com> on 2015/04/01 04:23:10 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/#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>.
> 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
>
>