----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57901/#review173011 -----------------------------------------------------------
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java Lines 97 (patched) <https://reviews.apache.org/r/57901/#comment246203> The previous code was serialized, this one isn't. It is most likely Ok because of the way clients are used now, but please 1) Comment this fact 2) Double-check that we do not have concurrent access to the client. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java Lines 100 (patched) <https://reviews.apache.org/r/57901/#comment246084> there *is* connection problem sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java Lines 110 (patched) <https://reviews.apache.org/r/57901/#comment246085> "Thrift call failed" sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java Lines 120 (patched) <https://reviews.apache.org/r/57901/#comment246086> How do we know that this is SentryHdfsServiceException? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java Lines 133 (patched) <https://reviews.apache.org/r/57901/#comment246087> can we reuse the message for log and exception? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java Lines 32 (patched) <https://reviews.apache.org/r/57901/#comment246208> This comment doesn't explain what connect() is doing and is talking about implementation. Please update to describe at high level what connect() is supposed to provide without referring to specific implementation. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClient.java Lines 39 (patched) <https://reviews.apache.org/r/57901/#comment246209> We do not want to add metrics on the interface, this comment probably should be fo rthe implementation parts. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java Line 82 (original), 74 (patched) <https://reviews.apache.org/r/57901/#comment246082> The factory has the correct config, why create it multiple times? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java Lines 83 (patched) <https://reviews.apache.org/r/57901/#comment246083> The factory already has the correct config, why create a new one? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java Lines 49 (patched) <https://reviews.apache.org/r/57901/#comment246081> You need to pass SentryPolicyClientTransportConfig() to SentryPolicyServiceClientDefaultImpl() as well - Alexander Kolbasov On April 26, 2017, 12:30 a.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57901/ > ----------------------------------------------------------- > > (Updated April 26, 2017, 12:30 a.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 9d18063 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryHdfsServiceException.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientInvocationHandler.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > 6cea596 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java > 636de40 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java > 12175f7 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java > 038bca7 > > 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/SentryTransportFactory.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyStoreConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/ThriftUtil.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 23552c2 > sentry-hdfs/sentry-hdfs-dist/pom.xml e828d5e > > 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 > 28b1224 > > 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 > 4dc99a2 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsServiceException.java > 307d8c3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java > 919b81b > > 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/SentryGenericServiceClientDefaultImpl.java > 075983e > > 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/persistent/TransactionManager.java > d11d34f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/PolicyStoreConstants.java > 8cf1c1a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 4284b53 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > fb73783 > > 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 > 2f38198 > > 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 > 8bda2f8 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericPolicyProcessor.java > d4bf435 > > 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 > 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/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 > 04d92dd > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java > 7292387 > > > Diff: https://reviews.apache.org/r/57901/diff/6/ > > > Testing > ------- > > > Thanks, > > kalyan kumar kalvagadda > >
