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/07 01:17:50 UTC
Re: Review Request 56954: SENTRY-1639 Refactor the usage of
configuration constants related to transport
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review168066
-----------------------------------------------------------
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 24 (patched)
<https://reviews.apache.org/r/56954/#comment240170>
The comment can say something like
"Configuration interface for Sentry Thrift clients.
The purpose of the interface is to abstract the knowledge of specific configuration keys and provide an API to extract various thrift-related configuration from a Config object"
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 27 (patched)
<https://reviews.apache.org/r/56954/#comment240171>
I am not sure we need these two kinds of members
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 33 (patched)
<https://reviews.apache.org/r/56954/#comment240177>
Extra line here and in all comments below
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 34 (patched)
<https://reviews.apache.org/r/56954/#comment240176>
conf isn't a sentry configuration - it may be hadoop configuration or Hive configuration.
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 46 (patched)
<https://reviews.apache.org/r/56954/#comment240174>
This one is unused - remove?
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 54 (patched)
<https://reviews.apache.org/r/56954/#comment240175>
change name to isKerberosEnabled()
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 59 (patched)
<https://reviews.apache.org/r/56954/#comment240179>
Ugi? It would be better to expand UGI to a full name
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 62 (patched)
<https://reviews.apache.org/r/56954/#comment240181>
isUgiTransportEnabled() to keep consistent with Kerberos
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 69 (patched)
<https://reviews.apache.org/r/56954/#comment240182>
Is it necessarily sentry principal? Would hdfs use sentry principal, for example?
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 84 (patched)
<https://reviews.apache.org/r/56954/#comment240183>
What is the relationship between addresses above and ports? For example, can we have two RPC addresses and one port? Can address include port or not? How would we configure two servers binding to the same address but different port?
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 94 (patched)
<https://reviews.apache.org/r/56954/#comment240184>
Include time units in the name and the doc
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 100 (patched)
<https://reviews.apache.org/r/56954/#comment240185>
As I mentioned before, we probably don't need this
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 106 (patched)
<https://reviews.apache.org/r/56954/#comment240186>
probably don't need this
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 112 (patched)
<https://reviews.apache.org/r/56954/#comment240187>
This looks a bit out of place here. Can this be refactored somehow so that it doesn't live with configuration?
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
Lines 33 (patched)
<https://reviews.apache.org/r/56954/#comment240188>
Seems that this isn't used anywhere.
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
Lines 59 (patched)
<https://reviews.apache.org/r/56954/#comment240191>
This can actually be an enum
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
Lines 118 (patched)
<https://reviews.apache.org/r/56954/#comment240192>
This can actually be an enum
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
Lines 35 (patched)
<https://reviews.apache.org/r/56954/#comment240193>
can be final
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
Lines 40 (patched)
<https://reviews.apache.org/r/56954/#comment240194>
You don't need this to be a singleton
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
Lines 39 (patched)
<https://reviews.apache.org/r/56954/#comment240195>
You don't need this to be a singleton
- Alexander Kolbasov
On Feb. 28, 2017, 1:24 a.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 1:24 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1639
> https://issues.apache.org/jira/browse/SENTRY-1639
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1639 Refactored the usage of configuration constants related to transport These are subset of changes done for SENTRY-1592.
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java PRE-CREATION
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java PRE-CREATION
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java PRE-CREATION
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java PRE-CREATION
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java 4ed1361
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java 03bf39e
> 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/service/thrift/SentryPolicyServiceClientDefaultImpl.java 2dc8af8
>
>
> Diff: https://reviews.apache.org/r/56954/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 56954: SENTRY-1639 Refactor the usage of
configuration constants related to transport
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
> > Lines 27 (patched)
> > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line27>
> >
> > I am not sure we need these two kinds of members
If we need to abstract configuration keys. Theere should be a way to get them as well.
In this case we are using them to in the generating error messges which need to have appropriate configuration key.
If not in this interface, we should be have another interface which expose the strings as well.
> On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
> > Lines 34 (patched)
> > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line34>
> >
> > conf isn't a sentry configuration - it may be hadoop configuration or Hive configuration.
To avaoid confusion, I have removed sentry keyword.
> On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
> > Lines 46 (patched)
> > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line46>
> >
> > This one is unused - remove?
This configuration is used when I work on connection pool. This refactored code is used by connection pool implmentation as well.
> On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
> > Lines 69 (patched)
> > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line69>
> >
> > Is it necessarily sentry principal? Would hdfs use sentry principal, for example?
Yes, sentry.hdfs.service.server.principal is the configuration. In the current design sentry.hdfs.service.server.principal and sentry.service.server.principal will be same.
> On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
> > Lines 84 (patched)
> > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line84>
> >
> > What is the relationship between addresses above and ports? For example, can we have two RPC addresses and one port? Can address include port or not? How would we configure two servers binding to the same address but different port?
port in RPC Addresses configured is optional. If a port is not provided for a server listed in RPC configuration, port configuration is used as a default port.
> On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
> > Lines 100 (patched)
> > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line100>
> >
> > As I mentioned before, we probably don't need this
If we need to abstract configuration keys. Theere should be a way to get them as well.
In this case we are using them to in the generating error messges which need to have appropriate configuration key.
If not in this interface, we should be have another interface which expose the strings as well.
> On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
> > Lines 106 (patched)
> > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line106>
> >
> > probably don't need this
If we need to abstract configuration keys. Theere should be a way to get them as well.
In this case we are using them to in the generating error messges which need to have appropriate configuration key.
If not in this interface, we should be have another interface which expose the strings as well.
> On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
> > Lines 112 (patched)
> > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line112>
> >
> > This looks a bit out of place here. Can this be refactored somehow so that it doesn't live with configuration?
Even this is a configuration. It is MAP of SASL configuration. As we use same set of SASL configuration we can move it a common place. I will make that change in the commit after this refactor commit.
> On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
> > Lines 40 (patched)
> > <https://reviews.apache.org/r/56954/diff/2/?file=1650804#file1650804line40>
> >
> > You don't need this to be a singleton
we talked about it before. I want to use polymorphic usage of SentryClientTransportConfigInterface.
> On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
> > Lines 39 (patched)
> > <https://reviews.apache.org/r/56954/diff/2/?file=1650805#file1650805line39>
> >
> > You don't need this to be a singleton
we talked about it before. I want to use polymorphic usage of SentryClientTransportConfigInterface.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review168066
-----------------------------------------------------------
On March 7, 2017, 6:51 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
>
> (Updated March 7, 2017, 6:51 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1639
> https://issues.apache.org/jira/browse/SENTRY-1639
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1639 Refactored the usage of configuration constants related to transport These are subset of changes done for SENTRY-1592.
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java PRE-CREATION
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java PRE-CREATION
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java PRE-CREATION
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java PRE-CREATION
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java 4ed1361
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java 03bf39e
> 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/service/thrift/SentryPolicyServiceClientDefaultImpl.java 2dc8af8
>
>
> Diff: https://reviews.apache.org/r/56954/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>