----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59167/#review175145 -----------------------------------------------------------
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java Lines 127 (patched) <https://reviews.apache.org/r/59167/#comment248547> typo "Do not limit" sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java Lines 135 (patched) <https://reviews.apache.org/r/59167/#comment248569> lifo is is set to ture by default. Should that be set explicitly? Can you explain what does it actuall do? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java Lines 148 (patched) <https://reviews.apache.org/r/59167/#comment248566> This logic creates a pool of connections. These connections could be spread accorss the servers that are up. Connections may or man not be evenly balanced accross the servers. Is this intent? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java Lines 161 (patched) <https://reviews.apache.org/r/59167/#comment248568> There is a confguration "sentry.service.client.connection.full.retry-total" that we had. This configuration is decides number of times to loop the complete list of servers when all the servers are down. This retry logic is removed now. You may want to add it. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java Lines 165 (patched) <https://reviews.apache.org/r/59167/#comment248571> when borrow object is called, it return an idle connection if it exists, if not it creates one. If the server with address "addr" is down the underneath poolFactory throws TTransportException. What does pool return in that case? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java Lines 201 (patched) <https://reviews.apache.org/r/59167/#comment248567> now that you have seperate the client from the connection. It should be easy to retian the connection even when the client is closed and re-use it. How about creating a pool of size 1 when the pool is disabled? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java Lines 220 (patched) <https://reviews.apache.org/r/59167/#comment248573> Here you are invalidating the complete pool. You are removed pooled instances with the specified key. You may want to update the comment. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java Lines 63 (patched) <https://reviews.apache.org/r/59167/#comment248534> As we do not have specific TransportFactory for specific clients, why should we expose it to client factory? Why not have it as a implementation detail of the SentryTransportPool? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java Line 89 (original), 91 (patched) <https://reviews.apache.org/r/59167/#comment248520> You have same function name getTransport to get TTransportWrapper from pool and TTransport from TTransportWrapper. You may want to rename them. It is confusing. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java Lines 72 (patched) <https://reviews.apache.org/r/59167/#comment248533> As we do not have specific TransportFactory for specific clients, why should we expose it to client factory? Why not have it as a implementation detail of the SentryTransportPool? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java Lines 66 (patched) <https://reviews.apache.org/r/59167/#comment248535> As we do not have specific TransportFactory for specific clients, why should we expose it to client factory? Why not have it as a implementation detail of the SentryTransportPool? - kalyan kumar kalvagadda On May 13, 2017, 10:01 p.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59167/ > ----------------------------------------------------------- > > (Updated May 13, 2017, 10:01 p.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 > 651173ebb7df995ff0c432e2ebec08b4d95c3a77 > > 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/transport/TransportFactory.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/4/ > > > Testing > ------- > > I used the interactive shell to test failover across two different servers. > > > Thanks, > > Alexander Kolbasov > >
