----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56134/#review170667 -----------------------------------------------------------
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/SentryServiceClient.java Lines 21 (patched) <https://reviews.apache.org/r/56134/#comment243553> You are telling who uses it without explaining what it is sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/SentryServiceClient.java Lines 23 (patched) <https://reviews.apache.org/r/56134/#comment243557> I don't think this is the right interface and the right name for it. What are you trying to do? What is the primitive operation and its meaning? You want something that can connect. The fact that it does it with some retries is its implementation detail which is irrelevant for the interface. You want to say - hey - stop using the connection (close() or something similar) That's it. And getRetryCount() doesn't belong here at all. So I think your interface should be something like this: interface Connector extends AutoCloseable { void connect() throws something; } sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/SentryServiceClient.java Lines 24 (patched) <https://reviews.apache.org/r/56134/#comment243558> In the patch it shows "extends Closeable" - IMO it should be AutoCloseable sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/SentryServiceClient.java Lines 41 (patched) <https://reviews.apache.org/r/56134/#comment243559> Since it extends Closeable, this shouldn't be here since it changes the interface (throwing out exception) - Alexander Kolbasov On Feb. 10, 2017, 9:44 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56134/ > ----------------------------------------------------------- > > (Updated Feb. 10, 2017, 9:44 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee > Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description > ------- > > There are couple of things done as part of this change set > 1. Extended the capability of sentry Namenode client and generic policy > client to connect to multiple servers and failover to the server that is > available > 2. Made design change so that logic that handles the transport layer of the > client to another class so that we don't have to duplicate the code and also > more maintainable. > 3. Moved the service constants need to handle the transport of these clients > separately. > > Note: Plese refer jira to look at the class diagram attached. > > > Diffs > ----- > > sentry-core/sentry-core-common/pom.xml 9d18063 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryClientInvocationHandler.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java > d321531 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/ThriftUtil.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/HdfsServiceTransportConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/PolicyServiceTransportConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/RetryClientInvocationHandler.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/SentryServiceClient.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryHdfsServiceException.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 23552c2 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java > ab12bf4 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java > 03bf39e > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java > 2a18b15 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java > db55b5a > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsServiceException.java > 307d8c3 > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/SentryHdfsServiceIntegrationBase.java > eccf83b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorWrapper.java > d320d0f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java > 11cdee7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java > ee6cdf7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java > 980d930 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java > f6bb8a5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > 1e72b74 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 2dc8af8 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryProcessorWrapper.java > a5f11a9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java > 5fed04a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java > d5f4fcb > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java > c4964c3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java > b8c7f23 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java > f822497 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 866ebc6 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java > 1ec8840 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactoryGM.java > dfae5ab > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java > 7292387 > > > Diff: https://reviews.apache.org/r/56134/diff/3/ > > > Testing > ------- > > I have tested the code using java client which use the client implementations. > > > Thanks, > > kalyan kumar kalvagadda > >
