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