> On May 17, 2017, 12:27 a.m., kalyan kumar kalvagadda wrote: > > 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/diff/4/?file=1717926#file1717926line135> > > > > lifo is is set to ture by default. Should that be set explicitly? Can > > you explain what does it actuall do?
Doesn't hurt to be explicit. We want to use the most recent connection first. > On May 17, 2017, 12:27 a.m., kalyan kumar kalvagadda wrote: > > 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/diff/4/?file=1717926#file1717926line148> > > > > 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? With pool it is rather difficult to achieve an even balancing. Do you think it is really important? We can try tp skew load balancing giving preference to a pool which has more idle connections, but this can disbalance load. I think we can fine-tune it once we get more practical experience. > On May 17, 2017, 12:27 a.m., kalyan kumar kalvagadda wrote: > > 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/diff/4/?file=1717926#file1717926line161> > > > > 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. I never understood why we have this setting in the first place - we do retry at the higher level by retrying client. Do you think it brings any value? > On May 17, 2017, 12:27 a.m., kalyan kumar kalvagadda wrote: > > 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/diff/4/?file=1717926#file1717926line165> > > > > 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? It will throw an exception and we will either continue to the next one or throw an exception to the caller > On May 17, 2017, 12:27 a.m., kalyan kumar kalvagadda wrote: > > 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/diff/4/?file=1717926#file1717926line201> > > > > 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? min and max can be tuned if needed. The pool is disabled for HDFS clients where this isn't needed. Otherwise we should just use the pool. > On May 17, 2017, 12:27 a.m., kalyan kumar kalvagadda wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java > > Lines 63 (patched) > > <https://reviews.apache.org/r/59167/diff/4/?file=1717933#file1717933line63> > > > > 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? I wanted to separate it for testing - there is a comment about it somewhere. This allows us to create test pool which doesn't connect to anything at all and creates fake objects. I have not done any specific tests yet - but that's the intention. > On May 17, 2017, 12:27 a.m., kalyan kumar kalvagadda wrote: > > 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/diff/4/?file=1717940#file1717940line72> > > > > 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? See the answer above. > On May 17, 2017, 12:27 a.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java > > Lines 66 (patched) > > <https://reviews.apache.org/r/59167/diff/4/?file=1717948#file1717948line67> > > > > 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? See the reply above. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59167/#review175145 ----------------------------------------------------------- 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 > >
