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