----------------------------------------------------------- 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 > >
