> On March 13, 2017, 8:25 p.m., Vadim Spector wrote: > > 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/diff/6/?file=1662746#file1662746line31> > > > > Is configfuration ever used as SentryClientTransportConfigInterface? If > > the code only deals with specific subclasses, what's the purpose of > > abstracting?
There is a lot of duplicate code in client that handles transport. In my nect commit, I will be moving all the common logic to a seperate class which needs this abstraction. > On March 13, 2017, 8:25 p.m., Vadim Spector wrote: > > 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/diff/6/?file=1662748#file1662748line35> > > > > 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? Yes, that was one of the reasons. I was not sure if that was a scenario that was possible. Wanted implemtation of SentryClientTransportConfigInterface to be a wrapper to get the required property from configuration provided. > On March 13, 2017, 8:25 p.m., Vadim Spector wrote: > > 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/diff/6/?file=1662752#file1662752line57> > > > > could you, please, add javadoc on thread safety of this class? Thrift clients are not thread safe. The client object could be shared used by multiple threads running at client side. All the public methods exposed to client application should be synchronized. Can we handle it as another jira? Let's not widen the scope of this commit. I need to get push the changes ASAP. > On March 13, 2017, 8:25 p.m., Vadim Spector wrote: > > 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/diff/6/?file=1662753#file1662753line61> > > > > could you, please, add javadoc on thread safety of this class? > > not obvious why some RPC APIs are synchronized while others are not. Thrift clients are not thread safe. The client object could be shared used by multiple threads running at client side. All the public methods exposed to client application should be synchronized. It should be fixed. Can we handle it as another jira? Let's not widen the scope of this commit. I need to get push the changes ASAP. > On March 13, 2017, 8:25 p.m., Vadim Spector wrote: > > 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/diff/6/?file=1662754#file1662754line79> > > > > could youi add javadoc on thread safety of this class? Thrift clients are not thread safe. The client object could be shared used by multiple threads running at client side. All the public methods exposed to client application should be synchronized. Can we handle it as another jira? Let's not widen the scope of this commit. I need to get push the changes ASAP. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56954/#review168795 ----------------------------------------------------------- On March 13, 2017, 11:47 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, 11:47 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/8/ > > > Testing > ------- > > > Thanks, > > kalyan kumar kalvagadda > >
