You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by kalyan kumar kalvagadda <kk...@cloudera.com> on 2017/04/03 15:18:34 UTC
Re: Review Request 57901: SENTRY-1593 Implement client failover for
all the sentry clients
> On March 30, 2017, 10:01 p.m., Lei Xu wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryHdfsServiceException.java
> > Lines 31 (patched)
> > <https://reviews.apache.org/r/57901/diff/2/?file=1680336#file1680336line31>
> >
> > Remove the blank lines.
Will fix
> On March 30, 2017, 10:01 p.m., Lei Xu wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
> > Lines 91 (patched)
> > <https://reviews.apache.org/r/57901/diff/2/?file=1680337#file1680337line91>
> >
> > Should we log the error?
When exception is thrown by connectWithRetry method, it is logged. Logging it here will be a duplicate lo again.
> On March 30, 2017, 10:01 p.m., Lei Xu wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/RetryClientInvocationHandler.java
> > Lines 111 (patched)
> > <https://reviews.apache.org/r/57901/diff/2/?file=1680337#file1680337line111>
> >
> > Should use `LOGGER.warnning` or `LOGGER.error` to be more visible in production.
Will fix
> On March 30, 2017, 10:01 p.m., Lei Xu wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientInvocationHandler.java
> > Lines 26 (patched)
> > <https://reviews.apache.org/r/57901/diff/2/?file=1680338#file1680338line26>
> >
> > Can we define this as `Closable` as well?
> >
> > So that it can be used in `try-resource` in JDK7.
Underneath client is already Closable. Do you still see any need for SentryClientInvocationHandler to be closable as well?
> On March 30, 2017, 10:01 p.m., Lei Xu wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClientTransportDefaultImpl.java
> > Lines 247 (patched)
> > <https://reviews.apache.org/r/57901/diff/2/?file=1680344#file1680344line247>
> >
> > `ConnectTo...` -> `connectTo...`
Will fix.
> On March 30, 2017, 10:01 p.m., Lei Xu 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>
> >
> > Can this be handled in the loop?
> > And here the `serverAddress` is not set, would it be problematic?
Agree, I have seperated this from the loop for a reason which is not valid now. I will address it.
> On March 30, 2017, 10:01 p.m., Lei Xu wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryServiceClientTransportDefaultImpl.java
> > Lines 269 (patched)
> > <https://reviews.apache.org/r/57901/diff/2/?file=1680344#file1680344line269>
> >
> > What will be thrown if `endpoints.isEmpty()`?
endpoints will never be empty. If the configuration from which this ArrayList is constrcuted is empty, exception will be thrown and the client object will fail.
> On March 30, 2017, 10:01 p.m., Lei Xu wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/ThriftUtil.java
> > Lines 32 (patched)
> > <https://reviews.apache.org/r/57901/diff/2/?file=1680346#file1680346line32>
> >
> > Please add some comments to this utility class.
This was existing file. I just moved it to another location. I will anyway add javadoc for this class.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57901/#review170644
-----------------------------------------------------------
On March 31, 2017, 10:28 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57901/
> -----------------------------------------------------------
>
> (Updated March 31, 2017, 10:28 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, 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/SentryServiceClientTransportDefaultImpl.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-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/SentryGenericServiceClient.java 11cdee7
> 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 fb7c40a
> 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/SentryPolicyServiceClient.java c2b03e5
> 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 ee2a466
> 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 d3a68c9
> 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/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/provider/db/service/thrift/TestSentryServiceFailureCase.java 7c7ebab
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java dfd79ae
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java 7292387
>
>
> Diff: https://reviews.apache.org/r/57901/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>