You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Colin Ma <ju...@intel.com> on 2015/01/05 04:28:34 UTC
Re: Review Request 29411: SENTRY-601: Create connection pool factory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29411/
-----------------------------------------------------------
(Updated Jan. 5, 2015, 3:28 a.m.)
Review request for sentry.
Repository: sentry
Description
-------
Create connection pool factory based on commons-pool 2.
Diffs (updated)
-----
pom.xml fa88493
sentry-provider/sentry-provider-db/pom.xml b9208ed
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
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/pool/PoolClientInvocationHandler.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolSentryServiceClientFactory.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java PRE-CREATION
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 be14afd
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 133daef
Diff: https://reviews.apache.org/r/29411/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 29411: SENTRY-601: Create connection pool factory
Posted by Colin Ma <ju...@intel.com>.
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java, line 43
> > <https://reviews.apache.org/r/29411/diff/3/?file=808364#file808364line43>
> >
> > also comment on thread safety. (I don't think this is thread safe).
Comment added, for the thread safety, the commons-pool will manage the connection pool, and every thread can get the connection by borrowObject() and return the connection to the pool by returnObject().
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java, line 78
> > <https://reviews.apache.org/r/29411/diff/3/?file=808364#file808364line78>
> >
> > nit: can't know -> don't know
done.
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java, line 108
> > <https://reviews.apache.org/r/29411/diff/3/?file=808364#file808364line108>
> >
> > This doesn't seem thread safe - what if another client adds a connection in after you do this check? I'm not sure if that would cause any problems, but something to call out.
> >
> >
> > I also think that it makes this confusing to use the recursive calls to implement retry. Instead consider implement it as a top-level while loop:
> >
> > retryCount = 0
> > while (retryCount < ...) {
> > invokeImpl();
> > }
> >
> > Object invokeImpl() {
> > // do the dance of borrowing the connection.
> > }
For the thread safe, when add new connection to the pool, the thread will get the lock for pool first. Thanks for the suggestion.
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java, line 117
> > <https://reviews.apache.org/r/29411/diff/3/?file=808364#file808364line117>
> >
> > How do you know this is due to authorization failed? It is a SentryUserException.
You're right, the exception is not about the authorization. It should be thrown by thrift call, eg, SentryAccessDeniedException.
Update the comment in the source code.
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java, line 126
> > <https://reviews.apache.org/r/29411/diff/3/?file=808364#file808364line126>
> >
> > *return
Done, thanks.
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java, line 31
> > <https://reviews.apache.org/r/29411/diff/3/?file=808365#file808365line31>
> >
> > ... if sentry connection pooling is enabled.
Done, thanks.
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java, line 39
> > <https://reviews.apache.org/r/29411/diff/3/?file=808365#file808365line39>
> >
> > private final
Done, thanks.
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java, line 41
> > <https://reviews.apache.org/r/29411/diff/3/?file=808365#file808365line41>
> >
> > remove unused ctor
Done, thanks.
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java, line 56
> > <https://reviews.apache.org/r/29411/diff/3/?file=808365#file808365line56>
> >
> > It seems like this client type should not be used when POOL_ENABLED=false. CHange to a Preconditions check and remove the code in the "else"?
Done, thanks.
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java, line 66
> > <https://reviews.apache.org/r/29411/diff/3/?file=808365#file808365line66>
> >
> > createInvocationHandler
Done, thanks.
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java, line 40
> > <https://reviews.apache.org/r/29411/diff/3/?file=808366#file808366line40>
> >
> > Delete default unused ctor
Done, thanks.
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java, line 72
> > <https://reviews.apache.org/r/29411/diff/3/?file=808369#file808369line72>
> >
> > Mul -> MultipleRetries
Done, thanks.
> On Jan. 9, 2015, 12:55 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java, line 57
> > <https://reviews.apache.org/r/29411/diff/3/?file=808365#file808365line57>
> >
> > Do we need to create a new instance of this with ever call to getClient? Also, the code is getting a bit confusing (not related to this change) with all the factories. Why doesn't the
> >
> > SentryServiceClientFactory
> > SentryPoolingClientFactory
> > SentryHAClientFactory
> >
> > Can you log a JIRA to see if we can simplify this a bit?
Hi Lenni, this patch is a subtask of Sentry-296, and these factories are also included in the other subtasks. There is no need to create a new JIRA, and I'll try to refactor these factories in other subtask of Sentry-296. Thanks for the comments.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29411/#review67335
-----------------------------------------------------------
On Jan. 9, 2015, 3:07 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29411/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2015, 3:07 p.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Create connection pool factory based on commons-pool 2.
>
>
> Diffs
> -----
>
> pom.xml fa88493
> sentry-provider/sentry-provider-db/pom.xml b9208ed
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> 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/pool/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java PRE-CREATION
> 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 be14afd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 133daef
>
> Diff: https://reviews.apache.org/r/29411/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 29411: SENTRY-601: Create connection pool factory
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29411/#review67335
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
<https://reviews.apache.org/r/29411/#comment111331>
much better, thanks!
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java
<https://reviews.apache.org/r/29411/#comment111339>
also comment on thread safety. (I don't think this is thread safe).
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java
<https://reviews.apache.org/r/29411/#comment111337>
nit: can't know -> don't know
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java
<https://reviews.apache.org/r/29411/#comment111340>
This doesn't seem thread safe - what if another client adds a connection in after you do this check? I'm not sure if that would cause any problems, but something to call out.
I also think that it makes this confusing to use the recursive calls to implement retry. Instead consider implement it as a top-level while loop:
retryCount = 0
while (retryCount < ...) {
invokeImpl();
}
Object invokeImpl() {
// do the dance of borrowing the connection.
}
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java
<https://reviews.apache.org/r/29411/#comment111335>
How do you know this is due to authorization failed? It is a SentryUserException.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java
<https://reviews.apache.org/r/29411/#comment111336>
*return
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java
<https://reviews.apache.org/r/29411/#comment111332>
... if sentry connection pooling is enabled.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java
<https://reviews.apache.org/r/29411/#comment111343>
private final
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java
<https://reviews.apache.org/r/29411/#comment111342>
remove unused ctor
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java
<https://reviews.apache.org/r/29411/#comment111349>
It seems like this client type should not be used when POOL_ENABLED=false. CHange to a Preconditions check and remove the code in the "else"?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java
<https://reviews.apache.org/r/29411/#comment111353>
Do we need to create a new instance of this with ever call to getClient? Also, the code is getting a bit confusing (not related to this change) with all the factories. Why doesn't the
SentryServiceClientFactory
SentryPoolingClientFactory
SentryHAClientFactory
Can you log a JIRA to see if we can simplify this a bit?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java
<https://reviews.apache.org/r/29411/#comment111345>
createInvocationHandler
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java
<https://reviews.apache.org/r/29411/#comment111334>
Delete default unused ctor
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java
<https://reviews.apache.org/r/29411/#comment111344>
Mul -> MultipleRetries
- Lenni Kuff
On Jan. 7, 2015, 1:43 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29411/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2015, 1:43 a.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Create connection pool factory based on commons-pool 2.
>
>
> Diffs
> -----
>
> pom.xml fa88493
> sentry-provider/sentry-provider-db/pom.xml b9208ed
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> 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/pool/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java PRE-CREATION
> 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 be14afd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 133daef
>
> Diff: https://reviews.apache.org/r/29411/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 29411: SENTRY-601: Create connection pool factory
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29411/#review67533
-----------------------------------------------------------
Ship it!
Thanks for the updates, looks great now.
- Lenni Kuff
On Jan. 9, 2015, 3:07 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29411/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2015, 3:07 p.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Create connection pool factory based on commons-pool 2.
>
>
> Diffs
> -----
>
> pom.xml fa88493
> sentry-provider/sentry-provider-db/pom.xml b9208ed
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> 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/pool/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java PRE-CREATION
> 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 be14afd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 133daef
>
> Diff: https://reviews.apache.org/r/29411/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 29411: SENTRY-601: Create connection pool factory
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29411/#review67530
-----------------------------------------------------------
Ship it!
Ship It!
- Lenni Kuff
On Jan. 9, 2015, 3:07 p.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29411/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2015, 3:07 p.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Create connection pool factory based on commons-pool 2.
>
>
> Diffs
> -----
>
> pom.xml fa88493
> sentry-provider/sentry-provider-db/pom.xml b9208ed
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> 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/pool/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java PRE-CREATION
> 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 be14afd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 133daef
>
> Diff: https://reviews.apache.org/r/29411/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 29411: SENTRY-601: Create connection pool factory
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29411/
-----------------------------------------------------------
(Updated Jan. 9, 2015, 3:07 p.m.)
Review request for sentry.
Repository: sentry
Description
-------
Create connection pool factory based on commons-pool 2.
Diffs (updated)
-----
pom.xml fa88493
sentry-provider/sentry-provider-db/pom.xml b9208ed
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
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/pool/PoolClientInvocationHandler.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java PRE-CREATION
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 be14afd
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 133daef
Diff: https://reviews.apache.org/r/29411/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 29411: SENTRY-601: Create connection pool factory
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29411/#review67353
-----------------------------------------------------------
This looks really nice, it's getting close. Thanks!
- Lenni Kuff
On Jan. 7, 2015, 1:43 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29411/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2015, 1:43 a.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Create connection pool factory based on commons-pool 2.
>
>
> Diffs
> -----
>
> pom.xml fa88493
> sentry-provider/sentry-provider-db/pom.xml b9208ed
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> 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/pool/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java PRE-CREATION
> 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 be14afd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 133daef
>
> Diff: https://reviews.apache.org/r/29411/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 29411: SENTRY-601: Create connection pool factory
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29411/
-----------------------------------------------------------
(Updated Jan. 7, 2015, 1:43 a.m.)
Review request for sentry.
Repository: sentry
Description
-------
Create connection pool factory based on commons-pool 2.
Diffs (updated)
-----
pom.xml fa88493
sentry-provider/sentry-provider-db/pom.xml b9208ed
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
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/pool/PoolClientInvocationHandler.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java PRE-CREATION
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 be14afd
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 133daef
Diff: https://reviews.apache.org/r/29411/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 29411: SENTRY-601: Create connection pool factory
Posted by Colin Ma <ju...@intel.com>.
> On Jan. 5, 2015, 6:08 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java, line 61
> > <https://reviews.apache.org/r/29411/diff/2/?file=806340#file806340line61>
> >
> > I don't understand what this code is doing. Can you please try to simplify it, break it up, and add some comments?
> > Also, why do we even need this? Shouldn't most of this be handled by the backing connection pool? Is it mainly to wrap exceptions?
Sorry for the missing comments, and the comments are added now.
This is a recursive method for:
1. get the client from commons-pool.
2. do the thrift call.
if there has connection problem, call this method again if (retryCount < connectionRetryTotal), else throw the exception.
if the authorization is failed, just throw the exception.
3. return the client to the commons-pool
This wrap is just for the retry if the connection is broken, and the connection pool can't check if the connection is broken in the Kerberos environment.
> On Jan. 5, 2015, 6:08 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java, line 178
> > <https://reviews.apache.org/r/29411/diff/2/?file=806338#file806338line178>
> >
> > IMO, just because commons connection pool exposes a bunch of configuration options options doesn't mean Sentry needs to support all of these. It would be nice if we could just choose some sensible defaults for many of these and only allow things like pool size to be configureable.
> >
> > Note that you are implementing a new connection connection pool backed by the commons pool so you can define the behavior and need not expose the backing implmentation, but be sure to comment on the behaviour of our connection pool implementation.
> >
> >
> > Also, please comment that these configuration options are mapping to values in the commons connection pool.
The configurations for pool size is kept, the others are in default value and can't be configured now.
> On Jan. 5, 2015, 6:08 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java, line 29
> > <https://reviews.apache.org/r/29411/diff/2/?file=806342#file806342line29>
> >
> > Add class comment to public class.
> >
> > Can this just be called SentryServiceClientPool?
The comment is added.
For the class name, this class is for connection pool to manage the object, and this class is extended from BasePooledObjectFactory. I think the SentryServiceClientPoolFactory is ok.
> On Jan. 5, 2015, 6:08 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolSentryServiceClientFactory.java, line 29
> > <https://reviews.apache.org/r/29411/diff/2/?file=806341#file806341line29>
> >
> > Add class comment to public class.
> >
> > Maybe rename to SentryPoolingClientFactory?
The comment is added and the class name is updated.
> On Jan. 5, 2015, 6:08 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java, line 37
> > <https://reviews.apache.org/r/29411/diff/2/?file=806340#file806340line37>
> >
> > Add class comment for public class.
The comment is added.
> On Jan. 5, 2015, 6:08 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/SentryServiceClientProxyFactory.java, line 42
> > <https://reviews.apache.org/r/29411/diff/2/?file=806339#file806339line42>
> >
> > nit: move this to right above where it is first used.
Done.
> On Jan. 5, 2015, 6:08 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolSentryServiceClientFactory.java, line 72
> > <https://reviews.apache.org/r/29411/diff/2/?file=806341#file806341line72>
> >
> > Comment on whether is is safe to call close multiple times (ie what if you close a client that has already been closed).
The comment is added.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29411/#review66667
-----------------------------------------------------------
On Jan. 7, 2015, 1:43 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29411/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2015, 1:43 a.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Create connection pool factory based on commons-pool 2.
>
>
> Diffs
> -----
>
> pom.xml fa88493
> sentry-provider/sentry-provider-db/pom.xml b9208ed
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> 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/pool/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryPoolingClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java PRE-CREATION
> 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 be14afd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 133daef
>
> Diff: https://reviews.apache.org/r/29411/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 29411: SENTRY-601: Create connection pool factory
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29411/#review66667
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
<https://reviews.apache.org/r/29411/#comment110240>
IMO, just because commons connection pool exposes a bunch of configuration options options doesn't mean Sentry needs to support all of these. It would be nice if we could just choose some sensible defaults for many of these and only allow things like pool size to be configureable.
Note that you are implementing a new connection connection pool backed by the commons pool so you can define the behavior and need not expose the backing implmentation, but be sure to comment on the behaviour of our connection pool implementation.
Also, please comment that these configuration options are mapping to values in the commons connection pool.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/SentryServiceClientProxyFactory.java
<https://reviews.apache.org/r/29411/#comment110246>
nit: move this to right above where it is first used.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java
<https://reviews.apache.org/r/29411/#comment110247>
Add class comment for public class.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolClientInvocationHandler.java
<https://reviews.apache.org/r/29411/#comment110259>
I don't understand what this code is doing. Can you please try to simplify it, break it up, and add some comments?
Also, why do we even need this? Shouldn't most of this be handled by the backing connection pool? Is it mainly to wrap exceptions?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolSentryServiceClientFactory.java
<https://reviews.apache.org/r/29411/#comment110250>
Add class comment to public class.
Maybe rename to SentryPoolingClientFactory?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolSentryServiceClientFactory.java
<https://reviews.apache.org/r/29411/#comment110253>
Comment on whether is is safe to call close multiple times (ie what if you close a client that has already been closed).
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java
<https://reviews.apache.org/r/29411/#comment110255>
Add class comment to public class.
Can this just be called SentryServiceClientPool?
- Lenni Kuff
On Jan. 5, 2015, 3:28 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29411/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2015, 3:28 a.m.)
>
>
> Review request for sentry.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Create connection pool factory based on commons-pool 2.
>
>
> Diffs
> -----
>
> pom.xml fa88493
> sentry-provider/sentry-provider-db/pom.xml b9208ed
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> 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/pool/PoolClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/PoolSentryServiceClientFactory.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/factory/pool/SentryServiceClientPoolFactory.java PRE-CREATION
> 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 be14afd
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java 133daef
>
> Diff: https://reviews.apache.org/r/29411/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>