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