----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56954/#review166838 -----------------------------------------------------------
The commit message isn't right - it doesn't match the JIRA. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java (line 24) <https://reviews.apache.org/r/56954/#comment238912> The first sentense should briefly describe the purpose of the interface. So, in this case it should be something like "Generic configuration interface for Sentry thrift clients." sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java (line 26) <https://reviews.apache.org/r/56954/#comment238913> This comment doesn't seem right - I do not see such two kinds of members. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java (line 30) <https://reviews.apache.org/r/56954/#comment238914> Please document every API within the interface. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java (line 40) <https://reviews.apache.org/r/56954/#comment238924> What are these Name() things and why do you need them? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java (line 24) <https://reviews.apache.org/r/56954/#comment238915> The comment can be improved a bit - it mentions clients too many times. Please make a short first sentence describing the class in general terms and then elaborate in more details. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java (line 29) <https://reviews.apache.org/r/56954/#comment238916> javadoc? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java (line 53) <https://reviews.apache.org/r/56954/#comment238917> javadoc? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java (line 54) <https://reviews.apache.org/r/56954/#comment238919> Please document the constants whenever possible. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java (line 101) <https://reviews.apache.org/r/56954/#comment238918> javadoc? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java (line 32) <https://reviews.apache.org/r/56954/#comment238922> Usually there is no need to document what a class implements since it is obvious from the declaration below. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java (line 36) <https://reviews.apache.org/r/56954/#comment238920> Remove empty line sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java (line 42) <https://reviews.apache.org/r/56954/#comment238921> WHy do you need this? Is it singleton? Why? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java (line 30) <https://reviews.apache.org/r/56954/#comment238923> See comments from the similar HDFS class. - Alexander Kolbasov On Feb. 22, 2017, 9:53 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56954/ > ----------------------------------------------------------- > > (Updated Feb. 22, 2017, 9:53 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 Added code changes to refactor 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-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java > 03bf39e1bed10dcd706ef12c078305b6497874f4 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java > ee6cdf79f0cc96a5a72de8d79ed9c175e9aae9a3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 2dc8af8c54c5ee59c618926438f4aa91b35fd13f > > Diff: https://reviews.apache.org/r/56954/diff/ > > > Testing > ------- > > > Thanks, > > kalyan kumar kalvagadda > >
