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