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

Reply via email to