> On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java > > Lines 76-103 (original), 73-82 (patched) > > <https://reviews.apache.org/r/59167/diff/6-7/?file=1731247#file1731247line76> > > > > this block of code is not needed.
Why is it not needed? > On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > pom.xml > > Line 63 (original), 63 (patched) > > <https://reviews.apache.org/r/59167/diff/7/?file=1733501#file1733501line63> > > > > what additional capabilities does pool 2.4.2 bring to the table? Bug fixes. Is there any reason to use an old version? > On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java > > Lines 121-126 (original), 120 (patched) > > <https://reviews.apache.org/r/59167/diff/7/?file=1733504#file1733504line121> > > > > By removing this code execute function would throw the exception that > > it get from SentryServiceClientFactory.create method instead of throwing > > RuntimeException. Please make sure that it is fine. There is a Throwable catch later here so it will return a failure with the message from the exception > On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java > > Line 406 (original), 407-411 (patched) > > <https://reviews.apache.org/r/59167/diff/7/?file=1733505#file1733505line407> > > > > why is this case handled differntly? Why not use the try with resource > > block? This class is never used and should be deleted. No point in fixing it. > On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java > > Line 107 (original), 106 (patched) > > <https://reviews.apache.org/r/59167/diff/7/?file=1733510#file1733510line115> > > > > Currently RetryClientInvocationHandler attempts to retry only for > > TTransportException. This is not sufficient. > > > > We need to indentify all the possible exceptions and clasify which > > amoung them are retryable. I will open a jira to address this seperately. This is the same as the current code - let's handle this in a separate issue. > On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java > > Lines 182-199 (patched) > > <https://reviews.apache.org/r/59167/diff/7/?file=1733512#file1733512line187> > > > > I understand the pooling is enabled by default for HDFS clients.I feel, > > it should be allowed to be configured as well. > > > > If you agree with that we can remove all these constants and return > > false always when SentryHDFSClientTransportConfig is used. It is disabled by default for HDFS clients but it is configurable. Are you suggesting making this non-configurable? > On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java > > Lines 201 (patched) > > <https://reviews.apache.org/r/59167/diff/7/?file=1733512#file1733512line206> > > > > Do we want load balancing for HDFS cients as well? Is there any reason not to? > On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java > > Line 45 (original) > > <https://reviews.apache.org/r/59167/diff/7/?file=1733513#file1733513line45> > > > > With this change, there will not be two retry coonfigurations, right. Can you clarify your question? > On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java > > Lines 51 (patched) > > <https://reviews.apache.org/r/59167/diff/7/?file=1733517#file1733517line51> > > > > It will be usefull to have some logging in SentryTransportPool which > > capture the connection count per server address when ever new connection is > > created while borrowing a transport. Added debug logging > On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportPool.java > > Lines 177-180 (patched) > > <https://reviews.apache.org/r/59167/diff/7/?file=1733517#file1733517line177> > > > > For some of the exceptions client should retry. Retry invocation > > handler should retry. Example: if the exception were > > NoSuchElementException, retry invocation handler should retry and client > > might get connection to another server. We are doing this retry between pools here - there is a for loop walking across servers. > On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java > > Lines 35-40 (patched) > > <https://reviews.apache.org/r/59167/diff/7/?file=1733529#file1733529line35> > > > > Are these changes relevent to this commit. I remember seeing these > > changes in simililar lines while fixing test failures. They are needed since otherwise there are torubles with Kafla tests > On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java > > Lines 192 (patched) > > <https://reviews.apache.org/r/59167/diff/7/?file=1733529#file1733529line199> > > > > Are these changes relevent to this commit. I remember seeing these > > changes in simililar lines while fixing test failures. See above > On June 1, 2017, 8:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > > Line 118 (original), 99 (patched) > > <https://reviews.apache.org/r/59167/diff/7/?file=1733537#file1733537line124> > > > > Why is the syncronized keyword removed? > > when we know that these methods are not thread safe why not leave it > > syncronized? > > > > Same comments to all other cases as well. All these methods are only called via proxy and synchronization is done at the proxy level. There is no non-proxied access to these methods. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59167/#review176454 ----------------------------------------------------------- On May 26, 2017, 8:53 p.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59167/ > ----------------------------------------------------------- > > (Updated May 26, 2017, 8:53 p.m.) > > > Review request for sentry, Brian Towles, Hao Hao, kalyan kumar kalvagadda, Na > Li, Sergio Pena, 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 > 74aced271dc28aeb294844ec7c0c2b78a0cdb9a3 > > 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-dist/pom.xml > c002337153e9a4e781b0417cf4be6729bda84935 > > 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 > e23d13ba21b1042a7e73dd8807018fa9899be609 > > 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/SentryService.java > 9beb07b43b405725f6f5e9b5d0d5c36f22f4cad8 > > 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/7/ > > > Testing > ------- > > I used the interactive shell to test failover across two different servers. > > > Thanks, > > Alexander Kolbasov > >
