> On Feb. 4, 2017, 12:20 a.m., Vamsee Yarlagadda wrote: > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/SentryHdfsServiceIntegrationBase.java, > > lines 59-62 > > <https://reviews.apache.org/r/56134/diff/2/?file=1621706#file1621706line59> > > > > How come these two are just the same?
Older code was duplicate. I corrected it. > On Feb. 4, 2017, 12:20 a.m., Vamsee Yarlagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java, > > lines 61-65 > > <https://reviews.apache.org/r/56134/diff/2/?file=1621710#file1621710line61> > > > > Reason for commenting this out. We currently do not have pool implemented for this client. I have approoprate comments. > On Feb. 4, 2017, 12:20 a.m., Vamsee Yarlagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java, > > line 263 > > <https://reviews.apache.org/r/56134/diff/2/?file=1621694#file1621694line263> > > > > During my testing for HMSFollower, I noticed that transport always > > returns isOpen as "true" even if the other end dies. > > > > But when we try to access the transport for messages, it then throws > > "Broken pipe" error. I have seen that behavior. That is why I have added logic to try another server when such an error is encountered. When we see such transport errors there will be a TTransportException. when we receive this exception we are we try to connect to next server, > On Feb. 4, 2017, 12:20 a.m., Vamsee Yarlagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java, > > line 230 > > <https://reviews.apache.org/r/56134/diff/2/?file=1621694#file1621694line230> > > > > For consistency, can we make this method "synchronized" as well? It is > > any reentrant as long as it is the same thread which is accessing both > > connect and connectWithRetry? connect() method is not a public, so it not exposed. Secondly, connect() method is called in the constructors only. This function need not be syncronized. > On Feb. 4, 2017, 12:20 a.m., Vamsee Yarlagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java, > > lines 49-71 > > <https://reviews.apache.org/r/56134/diff/2/?file=1621694#file1621694line49> > > > > Spacing is off across all files. > > https://wiki.cloudera.com/display/engineering/Java+Style+Guide > > > > 2 space for all new lines. > > 4 space for all continuation lines. > > > > Should be an easy fix once you set up your IDE configuration. Thanks for thre reference. > On Feb. 4, 2017, 12:20 a.m., Vamsee Yarlagadda wrote: > > sentry-core/sentry-core-common/pom.xml, lines 117-122 > > <https://reviews.apache.org/r/56134/diff/2/?file=1621692#file1621692line117> > > > > Do we need this? I don't see us overriding the default configuration so > > we probably might not need it? I have added to skip some of the file in my development. It is not needeed. I will remove it. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56134/#review164166 ----------------------------------------------------------- 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/ > > > Testing > ------- > > I have tested the code using java client which use the client implementations. > > > Thanks, > > kalyan kumar kalvagadda > >
