> On April 6, 2017, 1:58 p.m., Na Li wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java > > Lines 86 (patched) > > <https://reviews.apache.org/r/57901/diff/2/?file=1680337#file1680337line86> > > > > At line 64, you did not check if client_object is null or not. Is it > > acceptable when this input is null? When you call it at line 86 and if > > client is null, will it cause exception and terminate the retry?
If the client construction fails exception is thrown and RetryClientInvocationHandler will not be constructed at all. > On April 6, 2017, 1:58 p.m., Na Li wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClientTransportDefaultImpl.java > > Lines 207 (patched) > > <https://reviews.apache.org/r/57901/diff/2/?file=1680344#file1680344line207> > > > > should serverAddress be added into endpoints even when there is only > > one server? To make the logic uniform I have removed serverAddress member from the class. > On April 6, 2017, 1:58 p.m., Na Li wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClientTransportDefaultImpl.java > > Lines 249 (patched) > > <https://reviews.apache.org/r/57901/diff/2/?file=1680344#file1680344line249> > > > > line 207 set endpoints as null. so null exception could happen here. o make the logic uniform I have removed serverAddress member from the class and endpoints is populated always. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57901/#review170725 ----------------------------------------------------------- On April 18, 2017, 11:18 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57901/ > ----------------------------------------------------------- > > (Updated April 18, 2017, 11:18 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Vamsee > Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1593 > https://issues.apache.org/jira/browse/SENTRY-1593 > > > Repository: sentry > > > Description > ------- > > At a high level the change set submitted does following. > 1. Extends the capability of sentry name-node client and generic policy > client to connect to multiple servers and failover to the server that is > available > 2. Transport handling of the clients is abstracted to another class so that > the client's just have to extend the class that abstracts it. Implementations > of the clients don't have to worry about it. > 3. Changed the behavior of the clients so that they connect to one of the > configured servers when the client is actually instantiated. > > As part of the code changes there are couple of files that are moved from one > package to another. Here is the list of them > 1. RetryClientInvocationHandler.java > 2. SentryClientInvocationHandler.java > 3. ThriftUtil.java > 4. SentryHdfsServiceException.java > Amount these files, RetryClientInvocationHandler.java has changed. Please > have a closer look at this file. > > > Diffs > ----- > > sentry-core/sentry-core-common/pom.xml > 9d180635ca5ed6fb1def4ffdc4addda5a650593e > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransport.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java > 4ed1361fb1d79620c3098355a0ea9c27203a75de > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 23552c2512902a8500bfacb1c745ca4b10498cc8 > sentry-hdfs/sentry-hdfs-dist/pom.xml > beda2029616538101dc368b3d272fdfc942868ad > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java > 085971b34e3901b7a1d59bd8e7516b25f81ca872 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java > ab12bf402a9a04b28e2c6c06d4e55a3607132ead > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java > 03bf39e1bed10dcd706ef12c078305b6497874f4 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java > 2a18b1524546840df9e3275bd8a8220cc9de1b1f > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java > 4dc99a2bb6ec748a42ae2b65e937f6dec492fc48 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsServiceException.java > 307d8c3172533fa768145d23664ab536dcadd6b3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java > 919b81bd657e00841f4c8bb63a8851d1a44f8b78 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorWrapper.java > d320d0fdb827bfcf33814a3ffa56c1898edd2521 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java > ee6cdf79f0cc96a5a72de8d79ed9c175e9aae9a3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java > 980d9306d848d54fd476252793afc4c3a7fc6a08 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java > f6bb8a522217bc26d768026f1c6be8c3e758fad8 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java > fb7c40a4d32f6c5588b277c637e041ca8b3d999e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/PolicyStoreConstants.java > 8cf1c1a16a26939f4cf68278c7b8204ec0b08ca7 > > 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 > 2cf748e1e538c99fc34c57ed50b78701977c84ad > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > ee2a4662c1c564dda8d050340e6b06cdce516530 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryProcessorWrapper.java > a5f11a98f1bdb5bc9bd6c2c86b6fa40413b94bca > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java > 5fed04ad079869c77d7b60085db6ba7791f586ab > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java > d5f4fcb56d05dc699634927106f6fa31e74fa213 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java > c4964c34f6714c96d578bed49efc68d7f13e5925 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java > b8c7f23232f887b9c00662f7cef01e1a72c56eb7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java > f822497d9fdc8536d4bb9f52fa3d0f69acf78908 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > d3a68c9696d12cf353b59aa9b877f7ee971caf25 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java > d4bf4359c0d057f441ddd1f738f5468eaee5e385 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceClient.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java > 1ec884041b361df90e7186f05d0c964e1ba89fc4 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactoryGM.java > dfae5abaf6f3e52ca7d0a3a7862b0d39b6445016 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyServiceClient.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyStoreProcessor.java > 04d92ddc56932e49e555c88136255ce234738dbe > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java > dfd79aeb998d32f1f50e14328977ea1946d13029 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java > 729238707b9b78776e9db996de104c8f13f843bf > > > Diff: https://reviews.apache.org/r/57901/diff/5/ > > > Testing > ------- > > > Thanks, > > kalyan kumar kalvagadda > >