> 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? > > kalyan kumar kalvagadda wrote: > 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. > > Alexander Kolbasov wrote: > I think the comment was about *documenting* thread safety, not actually > changing anything in the code
Most of the public methods in this client should be syncronized but, that is the case in the current code. If we docuemnt it the way it is, it send a wrong signal. We need to correct the code and then document it. I will open a jira for this to correct them and docuement them as well. > 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 118 (original), 125 (patched) > > <https://reviews.apache.org/r/56954/diff/6/?file=1662752#file1662752line125> > > > > Since MissingConfigurationException logic replaces Preconditions, would > > it make sense to derive MissingConfigurationException from > > IllegalArgumentException? this way no need to re-throw RuntimeException. We need to re thrown the exception either way as we need to decorate the exception message. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56954/#review168795 ----------------------------------------------------------- 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 > >
