> On May 12, 2017, 2:26 a.m., Vamsee Yarlagadda wrote: > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerBase.java > > Line 362 (original), 362 (patched) > > <https://reviews.apache.org/r/59167/diff/2/?file=1715177#file1715177line362> > > > > I wonder if this goes into the process log or the stdout/stderr? But > > better to use LOGGER i guess. > > > > And in addition to that, this will swallow the exception and not raise > > it.
We are no longer using this class, this was needed for it to pass compilation - it will be removed soon. > On May 12, 2017, 2:26 a.m., Vamsee Yarlagadda wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java > > Lines 374 (patched) > > <https://reviews.apache.org/r/59167/diff/2/?file=1715179#file1715179line374> > > > > Same comment as above. Same thing - this class is not used. > On May 12, 2017, 2:26 a.m., Vamsee Yarlagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java > > Line 99 (original), 110 (patched) > > <https://reviews.apache.org/r/59167/diff/2/?file=1715186#file1715186line111> > > > > Does this connection pooling create and keep the connections on demand > > basis? Or it tries to establish this number of connections before > > proceeding further? It will try to set up min idle connections and keep number of idle connections between min and max, but will allow more active connections if needed. We may need to play with these a bit to tune to best values. > On May 12, 2017, 2:26 a.m., Vamsee Yarlagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java > > Lines 155 (patched) > > <https://reviews.apache.org/r/59167/diff/2/?file=1715190#file1715190line322> > > > > Even though the rest of this class is ThreadSafe, this invocation of > > USerGroupInformation is not. If there was a case where there are > > differences in the conf supplied, this could result in a strange behavior. > > But it is highly unlikely that conf/ changes within the running process. The way we construct this it can't happen because transport factory has a fixed config for the pool. Some tests run with different configs and they create new pool with a new factory which has different config. For production run config never changes. > On May 12, 2017, 2:26 a.m., Vamsee Yarlagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java > > Lines 88 (patched) > > <https://reviews.apache.org/r/59167/diff/2/?file=1715191#file1715191line88> > > > > Just wondering, is this thread safe? It isn't, but this is used strictly for testing and debug logging - we have a single pool in production, so we don't care. Tests don't do this in parallel either. I'll add a comment that it is test-only. > On May 12, 2017, 2:26 a.m., Vamsee Yarlagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java > > Lines 94 (patched) > > <https://reviews.apache.org/r/59167/diff/2/?file=1715191#file1715191line94> > > > > Shouldn't this come at the end of this constructor? I removed this line altogether. > On May 12, 2017, 2:26 a.m., Vamsee Yarlagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java > > Lines 160 (patched) > > <https://reviews.apache.org/r/59167/diff/2/?file=1715191#file1715191line160> > > > > Will commons-pool ensure that the object being returned has valid > > active connection? It will not since we are not enabling pre-borrow check. Instead the caller will detect that the connection is broken and invalidate it. > On May 12, 2017, 2:26 a.m., Vamsee Yarlagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java > > Lines 250 (patched) > > <https://reviews.apache.org/r/59167/diff/2/?file=1715191#file1715191line250> > > > > nit. typo fixed - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59167/#review174745 ----------------------------------------------------------- On May 11, 2017, 7:17 a.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59167/ > ----------------------------------------------------------- > > (Updated May 11, 2017, 7:17 a.m.) > > > Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na > Li, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1580 > https://issues.apache.org/jira/browse/SENTRY-1580 > > > Repository: sentry > > > Description > ------- > > SENTRY-1580: Provide pooled client connection model with HA > > > Diffs > ----- > > pom.xml ad54cfda5e7dabf9ecadc8f8d9f0e3012596fa32 > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java > a3140f2e23188e0bc7b6d5521a2f457d090a937f > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerBase.java > 262db11677c1bb999265a5033971c06def9455ed > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java > da587d00aa6e4b2fb6d78e3a720464d25bdcb47c > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java > fdb6df44953b20dfe655c62d6cbd935b7ba15bf4 > > sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java > 48511145098215ce5d077a3d335ac659a2fbbf95 > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java > 240067370b434054addfff00b985fb8c94b2bad9 > > sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/binding/SqoopAuthBinding.java > 84a61cc0bb5a7d5ba6ca3d74d7bcd3e66a8f351b > sentry-core/sentry-core-common/pom.xml > e1be256e9a4b867215afaf94b93cae2cc5e417b1 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java > 34a594eb24ab9de8ddfa91494e9c2c7d794e1129 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > 9ea7185c7a4660a4455a1f6436942043a7f81519 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java > c0768f96d3e702b258db3c52a0cf4bff0f890e49 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java > 2d8082787935bf3111f917761db3bc2d99f59df3 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java > c97fe97f67ba6e3ee64785d8baed266db3f7daca > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java > 9a10ca5aea7fff8a97394bf325fcdfe4418f91a5 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java > 9b9f9e84848dad02ab40dff2fa0232cecdebb878 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/TTransportWrapper.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/ThriftUtil.java > d9fab86727919496fd932407b3abf1a9d879f7e0 > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryUpdater.java > 34caa0ea845fa3f57b16212367397b3525e5a785 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java > 11f6894820f1df3102114787fa5fe2c060619541 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java > 798bbef98b29af7c885c5ea747d77d2dad6c1693 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java > e350103cfc31b4930e2a0cfdeeabc5d6e3615c4b > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/SentryHdfsServiceIntegrationBase.java > eccf83bdf65db618a912835c88c4051b9b41b4df > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java > 75d2993a4a7a86b99f206a82b9fe751e79a04adb > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java > 134012d18d75d12e3ccce82dac8145bfc41609f5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java > 41708c371b86e000e94ac78247bfb3bfc6c4a252 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java > 11cdee7f1be13fae10028e7ab20e0a3b06e75237 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java > b7ac640ed4e616d886b801a0cddb4b7990e7601b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java > 46ac4a3348f50ab1c02e09f3e1eea59690392f4e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryConfigToolSolr.java > 404adb8fa612d5e1026a0cf96a65a98db4ca10c3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellKafka.java > d6d9014a90b9171d02363d0f9e651891ba8c531d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellSolr.java > 695c008c03c927b1126423bb61c52f5e3c5b18b9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > c2b03e5d62b88bd5c2f696af8f8872c42c02de9c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > d1a4d99f8080fc6178770986acdd101d6fe997f7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java > 1d0984671ab5014b0a0ace263b7f9967e013b7ee > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java > acf9b05281d8456ebdf77414dc7886f9aa4ef28f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java > 7db93101c7f7534ea2c6e67ae5d661cbca8411b1 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java > 0164fa6197822bf450754dc1fd116581c2321d0f > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java > a4dd8a65634ce3fb07952782fb8fdc4d99e7f3f9 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java > a2027750add889e31ec1bb9b2a287d046e6c6343 > > sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java > bead00309f392b916c1ef5983248dedce3b99521 > > sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java > 0b1ef68a55a1d40637f771759735fc0838525a40 > > sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java > 8a01e1c5c4ec58eded5621ad94ee28bc68d66c0b > > > Diff: https://reviews.apache.org/r/59167/diff/2/ > > > Testing > ------- > > > Thanks, > > Alexander Kolbasov > >
