> On Feb. 27, 2017, 12:52 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java, > > line 26 > > <https://reviews.apache.org/r/56954/diff/1/?file=1643765#file1643765line26> > > > > This comment doesn't seem right - I do not see such two kinds of > > members. > > kalyan kumar kalvagadda wrote: > We need to expose the configration names for priciple and rpc config name > for logging puposes. Methods getSentryPrincipalConfigName and > getSentryServerRpcConfigName are added for that.
Do we actually need to expose actual config name? Can we do the required checks in the get config implementation itself? It doesn't look like getSentryPrincipalConfigName and getSentryServerRpcConfigName methods are actually needed - you can report misconfiguration right where you fetch the config values > On Feb. 27, 2017, 12:52 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java, > > line 40 > > <https://reviews.apache.org/r/56954/diff/1/?file=1643765#file1643765line40> > > > > What are these Name() things and why do you need them? > > kalyan kumar kalvagadda wrote: > We need to expose the configration names for priciple and rpc config name > for logging puposes. Methods getSentryPrincipalConfigName and > getSentryServerRpcConfigName are added for that. See comment above. > On Feb. 27, 2017, 12:52 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java, > > line 42 > > <https://reviews.apache.org/r/56954/diff/1/?file=1643767#file1643767line42> > > > > WHy do you need this? Is it singleton? Why? > > kalyan kumar kalvagadda wrote: > I need to instantiate these classes to get polymorphic behaviour. This > single instance should server the purpose of multiple sentry clients. I made > it singleton so that it is not instantiated multiple times. Yes, you need to instantiate the class and a simple new will be sufficient - there is no need to complicate matters with a singleton (where you need to deal with concurrency as well). It is perfectly fine to instantiate it multiple times. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56954/#review166838 ----------------------------------------------------------- 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/ > > > Testing > ------- > > > Thanks, > > kalyan kumar kalvagadda > >
