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