> On March 9, 2017, 12:40 a.m., Alexander Kolbasov wrote: > > 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/diff/4/?file=1659368#file1659368line40> > > > > do we want to pass boolean flag telling whether the option is > > mandatory? This is a general question for this interface.
In our case the implementation of the interface should decide if the property is mandatory for that client, not the applicaiton using this interface. I feel, we should not have such an option in interface functions. > On March 9, 2017, 12:40 a.m., Alexander Kolbasov wrote: > > 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/diff/4/?file=1659368#file1659368line79> > > > > I had the comment earlier - please document how this port is used in > > combination with multiple addresses. I have clarified you but did not add it in the comment. Adressing it now. > On March 9, 2017, 12:40 a.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java > > Lines 119 (patched) > > <https://reviews.apache.org/r/56954/diff/4/?file=1659373#file1659373line119> > > > > You only use it in constructor, so it can be a private var in > > constructor For uniformity I have transportConfig at class level in all client implementations as well. wrapUgi used only once we should be good. How ever all this code will be changed in my next commit. > On March 9, 2017, 12:40 a.m., Alexander Kolbasov wrote: > > 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/diff/4/?file=1659374#file1659374line70> > > > > This is only used in constructor, so can be a local var For uniformity I have transportConfig at class level in all client implementations as well. wrapUgi used only once we should be good. How ever all this code will be changed in my next commit. > On March 9, 2017, 12:40 a.m., Alexander Kolbasov wrote: > > 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/diff/4/?file=1659375#file1659375line91> > > > > This can be a local variable in constructors, but you may need wrapUgi > > as a class-level var instance of SentryPolicyClientTransportConfig is used out side constructor as well. It should be at class-level. For uniformity I have transportConfig at class level in other client implementations as well. wrapUgi used only once we should be good. How ever all this code will be changed in my next commit. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56954/#review168380 ----------------------------------------------------------- On March 22, 2017, 11:05 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56954/ > ----------------------------------------------------------- > > (Updated March 22, 2017, 11:05 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 > 2cf748e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java > c4964c3 > > > Diff: https://reviews.apache.org/r/56954/diff/9/ > > > Testing > ------- > > > Thanks, > > kalyan kumar kalvagadda > >
