You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Alexander Kolbasov <ak...@gmail.com> on 2017/03/31 06:12:39 UTC
Re: Review Request 56134: SENTRY-1593 Implementing client failover for
Generic policy clients and namenode clients
-----------------------------------------------------------
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
>
>