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/02/28 01:14:48 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/
-----------------------------------------------------------

(Updated Feb. 28, 2017, 1:14 a.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee Yarlagadda, and Vadim Spector.


Summary (updated)
-----------------

SENTRY-1639 Refactor the usage of configuration constants related to transport


Bugs: SENTRY-1639
    https://issues.apache.org/jira/browse/SENTRY-1639


Repository: sentry


Description
-------

SENTRY-1639 Added code changes to refactor 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/


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
> 
>

Re: Review Request 56954: SENTRY-1639 Refactor the usage of configuration constants related to transport

Posted by Alexander Kolbasov <ak...@gmail.com>.

-----------------------------------------------------------
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 9, 2017, 12:40 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
> > Lines 40 (patched)
> > <https://reviews.apache.org/r/56954/diff/4/?file=1659368#file1659368line40>
> >
> >     do we want to pass boolean flag telling whether the option is mandatory? This is a general question for this interface.

In our case the implementation of the interface should decide if the property is mandatory for that client, not the applicaiton using this interface.
I feel, we should not have such an option in interface functions.


> On March 9, 2017, 12:40 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
> > Lines 79 (patched)
> > <https://reviews.apache.org/r/56954/diff/4/?file=1659368#file1659368line79>
> >
> >     I had the comment earlier - please document how this port is used in combination with multiple addresses.

I have clarified you but did not add it in the comment. Adressing it now.


> On March 9, 2017, 12:40 a.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
> > Lines 119 (patched)
> > <https://reviews.apache.org/r/56954/diff/4/?file=1659373#file1659373line119>
> >
> >     You only use it in constructor, so it can be a private var in constructor

For uniformity I have transportConfig at class level in all client implementations as well.

wrapUgi used only once we should be good. How ever all this code will be changed in my next commit.


> On March 9, 2017, 12:40 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
> > Lines 68 (patched)
> > <https://reviews.apache.org/r/56954/diff/4/?file=1659374#file1659374line70>
> >
> >     This is only used in constructor, so can be a local var

For uniformity I have transportConfig at class level in all client implementations as well.

wrapUgi used only once we should be good. How ever all this code will be changed in my next commit.


> On March 9, 2017, 12:40 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
> > Lines 89 (patched)
> > <https://reviews.apache.org/r/56954/diff/4/?file=1659375#file1659375line91>
> >
> >     This can be a local variable in constructors, but you may need wrapUgi as a class-level var

instance of SentryPolicyClientTransportConfig is used out side constructor as well. It should be at class-level. For uniformity I have transportConfig at class level in other client implementations as well.

wrapUgi used only once we should be good. How ever all this code will be changed in my next commit.


- kalyan kumar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review168380
-----------------------------------------------------------


On March 22, 2017, 11:05 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
> 
> (Updated March 22, 2017, 11:05 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/exception/MissingConfigurationException.java PRE-CREATION 
>   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/MetastorePlugin.java 085971b 
>   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 2cf748e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 
> 
> 
> Diff: https://reviews.apache.org/r/56954/diff/9/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 56954: SENTRY-1639 Refactor the usage of configuration constants related to transport

Posted by Alexander Kolbasov <ak...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review168380
-----------------------------------------------------------




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java
Lines 21 (patched)
<https://reviews.apache.org/r/56954/#comment240590>

    Signals that a mandatory property is missing from the configuration



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java
Lines 25 (patched)
<https://reviews.apache.org/r/56954/#comment240591>

    Our convention is to use camelCase - configParam in this case



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 28 (patched)
<https://reviews.apache.org/r/56954/#comment240593>

    Extra " in the end



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 35 (patched)
<https://reviews.apache.org/r/56954/#comment240594>

    before giving up



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 40 (patched)
<https://reviews.apache.org/r/56954/#comment240595>

    do we want to pass boolean flag telling whether the option is mandatory? This is a general question for this interface.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 44 (patched)
<https://reviews.apache.org/r/56954/#comment240598>

    here and in other places you can shorten this with
    
    true iff kerberos should be enabled.
    
    iff stands for "if and only if"



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 49 (patched)
<https://reviews.apache.org/r/56954/#comment240596>

    isKerberosEnabled



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 53 (patched)
<https://reviews.apache.org/r/56954/#comment240597>

    Udi or Ugi?



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/#comment240599>

    @return name of Sentry server principal file (or is it something else?)



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 70 (patched)
<https://reviews.apache.org/r/56954/#comment240600>

    return comma-separated list of available Sentry Server addresses.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 79 (patched)
<https://reviews.apache.org/r/56954/#comment240601>

    I had the comment earlier - please document how this port is used in combination with multiple addresses.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 90 (patched)
<https://reviews.apache.org/r/56954/#comment240603>

    to another available server



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/#comment240602>

    getServerRpcConnTimeoutMs



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 99 (patched)
<https://reviews.apache.org/r/56954/#comment240604>

    So you decided to keep this for now?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
Lines 23 (patched)
<https://reviews.apache.org/r/56954/#comment240605>

    Extra space here and on the next line



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
Lines 24 (patched)
<https://reviews.apache.org/r/56954/#comment240606>

    Unused import?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
Lines 65 (patched)
<https://reviews.apache.org/r/56954/#comment240612>

    I think this changes the default from "true" to "false"



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
Lines 71 (patched)
<https://reviews.apache.org/r/56954/#comment240608>

    Why catch an exception here? You can drop precomdition and check for null itself.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
Lines 81 (patched)
<https://reviews.apache.org/r/56954/#comment240607>

    Why catch an exception here? You can drop precomdition and check for null itself.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
Lines 66 (patched)
<https://reviews.apache.org/r/56954/#comment240613>

    This changes the default from "true" to "false"



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
Lines 72 (patched)
<https://reviews.apache.org/r/56954/#comment240609>

    Same comment about exceptions.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Lines 119 (patched)
<https://reviews.apache.org/r/56954/#comment240610>

    You only use it in constructor, so it can be a private var in constructor



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 122 (original), 125 (patched)
<https://reviews.apache.org/r/56954/#comment240611>

    formatting seems wrong



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 146 (original), 144 (patched)
<https://reviews.apache.org/r/56954/#comment240614>

    I think the getSaslProperties should be just defined in this class or just used inline.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Lines 153 (patched)
<https://reviews.apache.org/r/56954/#comment240616>

    Space after :



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
Lines 68 (patched)
<https://reviews.apache.org/r/56954/#comment240617>

    This is only used in constructor, so can be a local var



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Lines 89 (patched)
<https://reviews.apache.org/r/56954/#comment240619>

    This can be a local variable in constructors, but you may need wrapUgi as a class-level var



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Lines 165 (patched)
<https://reviews.apache.org/r/56954/#comment240618>

    space after :


- Alexander Kolbasov


On March 8, 2017, 6:41 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
> 
> (Updated March 8, 2017, 6:41 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/exception/MissingConfigurationException.java PRE-CREATION 
>   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/4/
> 
> 
> 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 11, 2017, 12:12 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java
> > Lines 24 (patched)
> > <https://reviews.apache.org/r/56954/diff/5/?file=1661562#file1661562line24>
> >
> >     Do we need to define serialVersionUID?

It is needed of are serializing/desereliazing the object. In our case we are not serializing/desereliazing the exception object, so I did not add it.


> On March 11, 2017, 12:12 a.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
> > Lines 39 (patched)
> > <https://reviews.apache.org/r/56954/diff/5/?file=1661565#file1661565line39>
> >
> >     Here we trim the value before comparing, but in generic impl we don't. Is there a reason for treating it differently?

There is no such reason. I'm fixing it


- kalyan kumar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review168685
-----------------------------------------------------------


On March 22, 2017, 11:05 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
> 
> (Updated March 22, 2017, 11:05 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/exception/MissingConfigurationException.java PRE-CREATION 
>   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/MetastorePlugin.java 085971b 
>   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 2cf748e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 
> 
> 
> Diff: https://reviews.apache.org/r/56954/diff/9/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 56954: SENTRY-1639 Refactor the usage of configuration constants related to transport

Posted by Alexander Kolbasov <ak...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review168685
-----------------------------------------------------------




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java
Lines 24 (patched)
<https://reviews.apache.org/r/56954/#comment240957>

    Do we need to define serialVersionUID?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
Lines 39 (patched)
<https://reviews.apache.org/r/56954/#comment240959>

    Here we trim the value before comparing, but in generic impl we don't. Is there a reason for treating it differently?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Lines 123 (patched)
<https://reviews.apache.org/r/56954/#comment240961>

    A better way is to use
    
      private static final ImmutableMap<String, String> SASL_PROPERTIES =
              ImmutableMap.of(Sasl.SERVER_AUTH, "true",
              Sasl.QOP, "auth-conf");



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Lines 164 (patched)
<https://reviews.apache.org/r/56954/#comment240963>

    Why is this an IOException? It looks more like a RuntimeException.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
Lines 70 (patched)
<https://reviews.apache.org/r/56954/#comment240964>

    It is better to use
    
      private static final ImmutableMap<String, String> SASL_PROPERTIES =
              ImmutableMap.of(Sasl.SERVER_AUTH, "true",
                      Sasl.QOP, "auth-conf");



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Lines 93 (patched)
<https://reviews.apache.org/r/56954/#comment240965>

    Same comment about immutable map



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Lines 155 (patched)
<https://reviews.apache.org/r/56954/#comment240966>

    Can be moved inside try block



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Lines 189 (patched)
<https://reviews.apache.org/r/56954/#comment240967>

    Same comment about IOException


- Alexander Kolbasov


On March 10, 2017, 8:15 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
> 
> (Updated March 10, 2017, 8:15 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/exception/MissingConfigurationException.java PRE-CREATION 
>   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/5/
> 
> 
> 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 14, 2017, 3:10 a.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
> > Line 118 (original), 125 (patched)
> > <https://reviews.apache.org/r/56954/diff/8/?file=1662924#file1662924line125>
> >
> >     WHy did you change this from IOException to Exception? You are now using RuntimeError which is unchecked exception.

Will fix it.


> On March 14, 2017, 3:10 a.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
> > Lines 156 (patched)
> > <https://reviews.apache.org/r/56954/diff/8/?file=1662924#file1662924line161>
> >
> >     Can we just make MissingConfigurationException an unchecked exception (e.g. derived from RuntimeException) and just throw it? Then we don't even need to catch it

We still need to catch it to decorate the exception meesage.


> On March 14, 2017, 3:10 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
> > Lines 164 (patched)
> > <https://reviews.apache.org/r/56954/diff/8/?file=1662925#file1662925line168>
> >
> >     Same comment about exceptions

We still need to catch it to decorate the exception meesage.


- kalyan kumar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review168858
-----------------------------------------------------------


On March 22, 2017, 11:05 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
> 
> (Updated March 22, 2017, 11:05 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/exception/MissingConfigurationException.java PRE-CREATION 
>   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/MetastorePlugin.java 085971b 
>   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 2cf748e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 
> 
> 
> Diff: https://reviews.apache.org/r/56954/diff/9/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 56954: SENTRY-1639 Refactor the usage of configuration constants related to transport

Posted by Alexander Kolbasov <ak...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review168858
-----------------------------------------------------------


Fix it, then Ship it!




Ship It!


sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java
Line 26 (original), 26 (patched)
<https://reviews.apache.org/r/56954/#comment241140>

    Why not put quotes inside existing strings or use single quotes for clarity?
    "Property '" + configParam + "' is missing"



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 118 (original), 125 (patched)
<https://reviews.apache.org/r/56954/#comment241141>

    WHy did you change this from IOException to Exception? You are now using RuntimeError which is unchecked exception.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Lines 156 (patched)
<https://reviews.apache.org/r/56954/#comment241142>

    Can we just make MissingConfigurationException an unchecked exception (e.g. derived from RuntimeException) and just throw it? Then we don't even need to catch it



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
Lines 164 (patched)
<https://reviews.apache.org/r/56954/#comment241143>

    Same comment about exceptions



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Line 142 (original), 147 (patched)
<https://reviews.apache.org/r/56954/#comment241144>

    Same comment about Exception



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Line 197 (original), 202 (patched)
<https://reviews.apache.org/r/56954/#comment241145>

    Same comment regarding Exception



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java
Lines 97 (patched)
<https://reviews.apache.org/r/56954/#comment241147>

    What is the point of catching exception and immediately re-throwing it? You may simply not catch it.


- Alexander Kolbasov


On March 13, 2017, 11:47 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
> 
> (Updated March 13, 2017, 11:47 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/exception/MissingConfigurationException.java PRE-CREATION 
>   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/MetastorePlugin.java 085971b 
>   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 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 
> 
> 
> Diff: https://reviews.apache.org/r/56954/diff/8/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 56954: SENTRY-1639 Refactor the usage of configuration constants related to transport

Posted by Alexander Kolbasov <ak...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review169909
-----------------------------------------------------------


Ship it!




Ship It!

- Alexander Kolbasov


On March 23, 2017, 1:06 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
> 
> (Updated March 23, 2017, 1:06 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/exception/MissingConfigurationException.java PRE-CREATION 
>   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-dist/pom.xml beda202 
>   sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java 085971b 
>   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 2cf748e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 
> 
> 
> Diff: https://reviews.apache.org/r/56954/diff/10/
> 
> 
> 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>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/
-----------------------------------------------------------

(Updated March 23, 2017, 1:06 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee Yarlagadda, and Vadim Spector.


Changes
-------

Added change fixing SENTRY-1675 which was initially in SENTRY-1593


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 (updated)
-----

  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java PRE-CREATION 
  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-dist/pom.xml beda202 
  sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java 085971b 
  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 2cf748e 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 


Diff: https://reviews.apache.org/r/56954/diff/10/

Changes: https://reviews.apache.org/r/56954/diff/9-10/


Testing
-------


Thanks,

kalyan kumar kalvagadda

Re: Review Request 56954: SENTRY-1639 Refactor the usage of configuration constants related to transport

Posted by Alexander Kolbasov <ak...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review169846
-----------------------------------------------------------


Ship it!




The changes are good, but do you need to address SENTRY-1675?

- Alexander Kolbasov


On March 22, 2017, 11:05 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
> 
> (Updated March 22, 2017, 11:05 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/exception/MissingConfigurationException.java PRE-CREATION 
>   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/MetastorePlugin.java 085971b 
>   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 2cf748e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 
> 
> 
> Diff: https://reviews.apache.org/r/56954/diff/9/
> 
> 
> 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>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/
-----------------------------------------------------------

(Updated March 22, 2017, 11:05 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee Yarlagadda, and Vadim Spector.


Changes
-------

Comments provided bt vadim and sasha are addressed.


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 (updated)
-----

  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java PRE-CREATION 
  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/MetastorePlugin.java 085971b 
  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 2cf748e 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 


Diff: https://reviews.apache.org/r/56954/diff/9/

Changes: https://reviews.apache.org/r/56954/diff/8-9/


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>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/
-----------------------------------------------------------

(Updated March 13, 2017, 11:47 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee Yarlagadda, and Vadim Spector.


Changes
-------

Addressed review comments from Vadim


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 (updated)
-----

  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java PRE-CREATION 
  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/MetastorePlugin.java 085971b 
  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 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 


Diff: https://reviews.apache.org/r/56954/diff/8/

Changes: https://reviews.apache.org/r/56954/diff/7-8/


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>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/
-----------------------------------------------------------

(Updated March 13, 2017, 10:05 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee Yarlagadda, and Vadim Spector.


Changes
-------

Addressed vadim's comments.


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 (updated)
-----

  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java PRE-CREATION 
  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/MetastorePlugin.java 085971b 
  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 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 


Diff: https://reviews.apache.org/r/56954/diff/7/

Changes: https://reviews.apache.org/r/56954/diff/6-7/


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 13, 2017, 8:25 p.m., Vadim Spector wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
> > Lines 31 (patched)
> > <https://reviews.apache.org/r/56954/diff/6/?file=1662746#file1662746line31>
> >
> >     Is configfuration ever used as SentryClientTransportConfigInterface? If the code only deals with specific subclasses, what's the purpose of abstracting?

There is a lot of duplicate code in client that handles transport. In my nect commit, I will be moving all the common logic to a seperate class which needs this abstraction.


> On March 13, 2017, 8:25 p.m., Vadim Spector wrote:
> > 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/diff/6/?file=1662748#file1662748line35>
> >
> >     why not to to pass configuration once to the constructor instead of to each method? Is it because one instance of SentryHDFSClientTransportConfig may be used with multiple configurations?

Yes, that was one of the reasons. I was not sure if that was a scenario that was possible. Wanted implemtation of SentryClientTransportConfigInterface to be a wrapper to get the required property from configuration provided.


> On March 13, 2017, 8:25 p.m., Vadim Spector wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
> > Line 53 (original), 57 (patched)
> > <https://reviews.apache.org/r/56954/diff/6/?file=1662752#file1662752line57>
> >
> >     could you, please, add javadoc on thread safety of this class?

Thrift clients are not thread safe. The client object could be shared used by multiple threads running at client side. All the public methods exposed to client application should be synchronized.
Can we handle it as another jira? Let's not widen the scope of this commit. I need to get push the changes ASAP.


> On March 13, 2017, 8:25 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
> > Line 56 (original), 59 (patched)
> > <https://reviews.apache.org/r/56954/diff/6/?file=1662753#file1662753line61>
> >
> >     could you, please, add javadoc on thread safety of this class?
> >     not obvious why some RPC APIs are synchronized while others are not.

Thrift clients are not thread safe. The client object could be shared used by multiple threads running at client side. All the public methods exposed to client application should be synchronized.

It should be fixed. Can we handle it as another jira? Let's not widen the scope of this commit. I need to get push the changes ASAP.


> On March 13, 2017, 8:25 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
> > Line 75 (original), 77 (patched)
> > <https://reviews.apache.org/r/56954/diff/6/?file=1662754#file1662754line79>
> >
> >     could youi add javadoc on thread safety of this class?

Thrift clients are not thread safe. The client object could be shared used by multiple threads running at client side. All the public methods exposed to client application should be synchronized.
Can we handle it as another jira? Let's not widen the scope of this commit. I need to get push the changes ASAP.


- kalyan kumar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review168795
-----------------------------------------------------------


On March 13, 2017, 11:47 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
> 
> (Updated March 13, 2017, 11:47 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/exception/MissingConfigurationException.java PRE-CREATION 
>   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/MetastorePlugin.java 085971b 
>   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 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 
> 
> 
> Diff: https://reviews.apache.org/r/56954/diff/8/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 56954: SENTRY-1639 Refactor the usage of configuration constants related to transport

Posted by Alexander Kolbasov <ak...@gmail.com>.


> On March 13, 2017, 8:25 p.m., Vadim Spector wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
> > Line 53 (original), 57 (patched)
> > <https://reviews.apache.org/r/56954/diff/6/?file=1662752#file1662752line57>
> >
> >     could you, please, add javadoc on thread safety of this class?
> 
> kalyan kumar kalvagadda wrote:
>     Thrift clients are not thread safe. The client object could be shared used by multiple threads running at client side. All the public methods exposed to client application should be synchronized.
>     Can we handle it as another jira? Let's not widen the scope of this commit. I need to get push the changes ASAP.

I think the comment was about *documenting* thread safety, not actually changing anything in the code


- Alexander


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review168795
-----------------------------------------------------------


On March 13, 2017, 11:47 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
> 
> (Updated March 13, 2017, 11:47 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/exception/MissingConfigurationException.java PRE-CREATION 
>   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/MetastorePlugin.java 085971b 
>   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 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 
> 
> 
> Diff: https://reviews.apache.org/r/56954/diff/8/
> 
> 
> 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 13, 2017, 8:25 p.m., Vadim Spector wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
> > Line 53 (original), 57 (patched)
> > <https://reviews.apache.org/r/56954/diff/6/?file=1662752#file1662752line57>
> >
> >     could you, please, add javadoc on thread safety of this class?
> 
> kalyan kumar kalvagadda wrote:
>     Thrift clients are not thread safe. The client object could be shared used by multiple threads running at client side. All the public methods exposed to client application should be synchronized.
>     Can we handle it as another jira? Let's not widen the scope of this commit. I need to get push the changes ASAP.
> 
> Alexander Kolbasov wrote:
>     I think the comment was about *documenting* thread safety, not actually changing anything in the code

Most of the public methods in this client should be syncronized but, that is the case in the current code. If we docuemnt it the way it is, it send a wrong signal. We need to correct the code and then document it. I will open a jira for this to correct them and docuement them as well.


> On March 13, 2017, 8:25 p.m., Vadim Spector wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
> > Line 118 (original), 125 (patched)
> > <https://reviews.apache.org/r/56954/diff/6/?file=1662752#file1662752line125>
> >
> >     Since MissingConfigurationException logic replaces Preconditions, would it make sense to derive MissingConfigurationException from IllegalArgumentException? this way no need to re-throw RuntimeException.

We need to re thrown the exception either way as we need to decorate the exception message.


- kalyan kumar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review168795
-----------------------------------------------------------


On March 22, 2017, 11:05 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
> 
> (Updated March 22, 2017, 11:05 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/exception/MissingConfigurationException.java PRE-CREATION 
>   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/MetastorePlugin.java 085971b 
>   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 2cf748e 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 
> 
> 
> Diff: https://reviews.apache.org/r/56954/diff/9/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 56954: SENTRY-1639 Refactor the usage of configuration constants related to transport

Posted by Vadim Spector <vs...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/#review168795
-----------------------------------------------------------




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java
Lines 26 (patched)
<https://reviews.apache.org/r/56954/#comment241048>

    I'd recommend configParam printed in quotes, to make spaces in parameter names (if any) visible.



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java
Lines 31 (patched)
<https://reviews.apache.org/r/56954/#comment241063>

    Is configfuration ever used as SentryClientTransportConfigInterface? If the code only deals with specific subclasses, what's the purpose of abstracting?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java
Lines 111 (patched)
<https://reviews.apache.org/r/56954/#comment241058>

    In PolicyClientConstants and HDFSClientConstants there are several constants with the same name. Could you, please, define those constants at the beginning in both classes - and in the same order - for better readability?



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/#comment241064>

    why not to to pass configuration once to the constructor instead of to each method? Is it because one instance of SentryHDFSClientTransportConfig may be used with multiple configurations?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java
Lines 51 (patched)
<https://reviews.apache.org/r/56954/#comment241049>

    functionally ~ to Boolean.valueOf()



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
Lines 51 (patched)
<https://reviews.apache.org/r/56954/#comment241050>

    functionally ~ to Boolean.valueOf()



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java
Lines 60 (patched)
<https://reviews.apache.org/r/56954/#comment241062>

    Should be PRINCIPAL instead of SERVER_RPC_ADDRESS.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 53 (original), 57 (patched)
<https://reviews.apache.org/r/56954/#comment241069>

    could you, please, add javadoc on thread safety of this class?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java
Line 118 (original), 125 (patched)
<https://reviews.apache.org/r/56954/#comment241068>

    Since MissingConfigurationException logic replaces Preconditions, would it make sense to derive MissingConfigurationException from IllegalArgumentException? this way no need to re-throw RuntimeException.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
Line 56 (original), 59 (patched)
<https://reviews.apache.org/r/56954/#comment241070>

    could you, please, add javadoc on thread safety of this class?
    not obvious why some RPC APIs are synchronized while others are not.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Line 75 (original), 77 (patched)
<https://reviews.apache.org/r/56954/#comment241071>

    could youi add javadoc on thread safety of this class?


- Vadim Spector


On March 13, 2017, 4:35 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56954/
> -----------------------------------------------------------
> 
> (Updated March 13, 2017, 4:35 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/exception/MissingConfigurationException.java PRE-CREATION 
>   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/MetastorePlugin.java 085971b 
>   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 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 
> 
> 
> Diff: https://reviews.apache.org/r/56954/diff/6/
> 
> 
> 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>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/
-----------------------------------------------------------

(Updated March 13, 2017, 4:35 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee Yarlagadda, and Vadim Spector.


Changes
-------

Addressed review comments.


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 (updated)
-----

  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java PRE-CREATION 
  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/MetastorePlugin.java 085971b 
  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 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java c4964c3 


Diff: https://reviews.apache.org/r/56954/diff/6/

Changes: https://reviews.apache.org/r/56954/diff/5-6/


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>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/
-----------------------------------------------------------

(Updated March 10, 2017, 8:15 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee Yarlagadda, and Vadim Spector.


Changes
-------

Addressed reivew comments


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 (updated)
-----

  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java PRE-CREATION 
  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/5/

Changes: https://reviews.apache.org/r/56954/diff/4-5/


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>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56954/
-----------------------------------------------------------

(Updated March 8, 2017, 6:41 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee Yarlagadda, and Vadim Spector.


Changes
-------

Addressed sasha review comments


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 (updated)
-----

  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/MissingConfigurationException.java PRE-CREATION 
  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/4/

Changes: https://reviews.apache.org/r/56954/diff/3-4/


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>.

-----------------------------------------------------------
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.


Changes
-------

Incorporated review comments.


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 (updated)
-----

  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/

Changes: https://reviews.apache.org/r/56954/diff/2-3/


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>.

-----------------------------------------------------------
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 (updated)
-------

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/


Testing
-------


Thanks,

kalyan kumar kalvagadda