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