----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56134/#review164002 -----------------------------------------------------------
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 50) <https://reviews.apache.org/r/56134/#comment235508> I will update the comment. "Will have" is a wrong phrase. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 53) <https://reviews.apache.org/r/56134/#comment235509> There are no abstract methods in this class. Most of them are private except for couple of them. Theay are protected as they are used in child class. Some of the member varaibles can be made final. I will do that change. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 56) <https://reviews.apache.org/r/56134/#comment235510> Yes, Other wise we need to construct it every time client tries to connect. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 58) <https://reviews.apache.org/r/56134/#comment235511> transport is not initialized in the constructor so it can not be final. It is used in child class to the visbility is protected. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 59) <https://reviews.apache.org/r/56134/#comment235513> yes, will update it. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 64) <https://reviews.apache.org/r/56134/#comment235515> It need not be public. I will be changinh the visibility to protected. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 67) <https://reviews.apache.org/r/56134/#comment235516> Will correct it sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 70) <https://reviews.apache.org/r/56134/#comment235517> We need to do this if we need to re-use it in some other class. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 76) <https://reviews.apache.org/r/56134/#comment235518> constructor should be throws both of them. Intellij normally shows such things. I'm wondering why the editor did not point it. I will add it. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/HdfsServiceTransportConstants.java (line 31) <https://reviews.apache.org/r/56134/#comment235598> I will consider ir sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/HdfsServiceTransportConstants.java (line 48) <https://reviews.apache.org/r/56134/#comment235558> I'm using them as class varables. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 54) <https://reviews.apache.org/r/56134/#comment235522> Will address it sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 62) <https://reviews.apache.org/r/56134/#comment235549> I'm using ServiceTransportConstants to refer the child class instances. I took this apprach to acheive it. Please refer to the constructor of SentryServiceClientTransportDefaultImpl class. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 66) <https://reviews.apache.org/r/56134/#comment235550> I'm using ServiceTransportConstants to refer the child class instances. I took this apprach to acheive it. Please refer to the constructor of SentryServiceClientTransportDefaultImpl class. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 70) <https://reviews.apache.org/r/56134/#comment235551> Same as above sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 71) <https://reviews.apache.org/r/56134/#comment235552> Same as above. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 73) <https://reviews.apache.org/r/56134/#comment235554> These are implemeted in child classes. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 74) <https://reviews.apache.org/r/56134/#comment235555> will fix them sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 84) <https://reviews.apache.org/r/56134/#comment235557> This map is needed for UgiSaslClientTransport creation. sentry-dist/pom.xml (line 101) <https://reviews.apache.org/r/56134/#comment235548> Will remove it sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java (line 56) <https://reviews.apache.org/r/56134/#comment235519> pool logic is not implemented for SentryGenericServiceClient and SentryHdfsServiceClient Once we have the implementaion approprate object is constructed and returned. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java (line 58) <https://reviews.apache.org/r/56134/#comment235520> This was not old code. I will anyway remove it sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java (line 66) <https://reviews.apache.org/r/56134/#comment235521> Other apprach that we could take is by building a proative connecting logic by having a runnable task peorically checking the connection status of these client connections and update the connection state in the client object and also try to re-connect. That we don't have to estabish the connections when the client API's are called. - kalyan kumar kalvagadda On Feb. 1, 2017, 9:11 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56134/ > ----------------------------------------------------------- > > (Updated Feb. 1, 2017, 9:11 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee > Yarlagadda, and Vadim Spector. > > > Repository: sentry > > > Description > ------- > > There are couple of things done as part of this change set > 1. Extended the capability of sentry Namenode client and generic policy > client to connect to multiple servers and failover to the server that is > available > 2. Made design change so that logic that handles the transport layer of the > client to another class so that we don't have to duplicate the code and also > more maintainable. > 3. Moved the service constants need to handle the transport of these clients > separately. > > Note: Plese refer jira to look at the class diagram attached. > > > Diffs > ----- > > sentry-core/sentry-core-common/pom.xml > 9d180635ca5ed6fb1def4ffdc4addda5a650593e > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryMain.java > d321531de0537045c8ef97df6a99cf3b7fe48964 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/HdfsServiceTransportConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/PolicyServiceTransportConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/SentryServiceClient.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java > PRE-CREATION > sentry-dist/pom.xml 5952b879aae4a4410b707a8ba10637737d5b1149 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 23552c2512902a8500bfacb1c745ca4b10498cc8 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java > ab12bf402a9a04b28e2c6c06d4e55a3607132ead > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java > 03bf39e1bed10dcd706ef12c078305b6497874f4 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java > 2a18b1524546840df9e3275bd8a8220cc9de1b1f > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java > db55b5aa33cebbef235cceca6ccda48603da2a26 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsServiceException.java > 307d8c3172533fa768145d23664ab536dcadd6b3 > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/SentryHdfsServiceIntegrationBase.java > eccf83bdf65db618a912835c88c4051b9b41b4df > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessorWrapper.java > d320d0fdb827bfcf33814a3ffa56c1898edd2521 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java > 11cdee7f1be13fae10028e7ab20e0a3b06e75237 > > 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/generic/service/thrift/SentryGenericServiceClientFactory.java > 980d9306d848d54fd476252793afc4c3a7fc6a08 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java > f6bb8a522217bc26d768026f1c6be8c3e758fad8 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 0712e2c9246ba7fe0c81d42861628c60eee2cfa0 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > 1e72b74fcabc6ef5763f2020b57407a6efb640a9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 2dc8af8c54c5ee59c618926438f4aa91b35fd13f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryProcessorWrapper.java > a5f11a98f1bdb5bc9bd6c2c86b6fa40413b94bca > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/ThriftUtil.java > 5fed04ad079869c77d7b60085db6ba7791f586ab > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PoolClientInvocationHandler.java > d5f4fcb56d05dc699634927106f6fa31e74fa213 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/RetryClientInvocationHandler.java > c4964c34f6714c96d578bed49efc68d7f13e5925 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryClientInvocationHandler.java > b8c7f23232f887b9c00662f7cef01e1a72c56eb7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java > f822497d9fdc8536d4bb9f52fa3d0f69acf78908 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 866ebc659ef0e7b923f95cb6e311e137814da1f3 > > sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.derby.sql > 1883626262bf4f4936f156a7ac74365b9b5873df > > sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.mysql.sql > 1829e2fa1f02a4339e7af4bf45a169013e9ec65f > > sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.oracle.sql > 7de9751892a8ff84067f67d542ac58d33e9148d8 > > sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.postgres.sql > adf5f1f39596309183f8c80d2c8ad1f1a7730236 > sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql > 0606116a1d31c400fb6dadcd80a2284007117ab2 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql > be9a33e6b0904b7c8ec522906d967081631108d6 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql > 1c8848c30d0432ebdbd18b73a1d27c61d2b7bcae > > sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql > fc7b53f41e97fad5e5baeb254b686a0e8cc5b003 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql > 92d0e3314141e01cab3a8321bba0ca253378b27f > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java > 1ec884041b361df90e7186f05d0c964e1ba89fc4 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactoryGM.java > dfae5abaf6f3e52ca7d0a3a7862b0d39b6445016 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestPoolClientInvocationHandler.java > 729238707b9b78776e9db996de104c8f13f843bf > > Diff: https://reviews.apache.org/r/56134/diff/ > > > Testing > ------- > > I have tested the code using java client which use the client implementations. > > > Thanks, > > kalyan kumar kalvagadda > >
