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