----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56134/#review163862 -----------------------------------------------------------
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 20) <https://reviews.apache.org/r/56134/#comment235365> It is better to keep all constants together - ServiceConstants seems to be a good place, sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 25) <https://reviews.apache.org/r/56134/#comment235366> Please don't mix constant classes with abstract classes. sentry-dist/pom.xml (line 101) <https://reviews.apache.org/r/56134/#comment235343> Please remove this change. Current situation requires users to install JDBC connectors themselves. I don't think we can change it in the context of this JIRA. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 50) <https://reviews.apache.org/r/56134/#comment235393> What do you mean by "will have"? It has these methods or they will be implemented in the future? Please provide more detailed description of the class. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 53) <https://reviews.apache.org/r/56134/#comment235392> Are there any abstract methods in this class? Please check whether any fields can be made final and what should be the visibility for these fields - do we really need anything other than private? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 54) <https://reviews.apache.org/r/56134/#comment235391> Sentry code uses 2-space indent Does it really needs to be protected? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 56) <https://reviews.apache.org/r/56134/#comment235394> Do you need this as class field? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 58) <https://reviews.apache.org/r/56134/#comment235395> Can it be final? Why is it protected? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 59) <https://reviews.apache.org/r/56134/#comment235396> Can this be final? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 64) <https://reviews.apache.org/r/56134/#comment235400> Why is this public? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 67) <https://reviews.apache.org/r/56134/#comment235390> Can you check formatting for this file? Indents seem to be off sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 70) <https://reviews.apache.org/r/56134/#comment235402> I think we should extract this to stand-alone file/class sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 76) <https://reviews.apache.org/r/56134/#comment235401> COnstructor throws SaslException rather than IOException sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/HdfsServiceTransportConstants.java (line 31) <https://reviews.apache.org/r/56134/#comment235387> I don't think it is a good idea to extend constant classes. It could be better to just split constants into separate classes. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/HdfsServiceTransportConstants.java (line 48) <https://reviews.apache.org/r/56134/#comment235388> Should all these be static? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 25) <https://reviews.apache.org/r/56134/#comment235376> Please document all members of the class. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 54) <https://reviews.apache.org/r/56134/#comment235377> Please update the comment with correct links. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 62) <https://reviews.apache.org/r/56134/#comment235378> Why do you need access methods for constants? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 66) <https://reviews.apache.org/r/56134/#comment235379> Ditto sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 70) <https://reviews.apache.org/r/56134/#comment235380> Ditto sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 71) <https://reviews.apache.org/r/56134/#comment235381> Ditto sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 73) <https://reviews.apache.org/r/56134/#comment235382> What are all these? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 74) <https://reviews.apache.org/r/56134/#comment235385> The usual order is public abstract sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (line 84) <https://reviews.apache.org/r/56134/#comment235384> What is this? sentry-dist/pom.xml (line 101) <https://reviews.apache.org/r/56134/#comment235368> See earlier comment about this change. 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/#comment235404> Why return null here? 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/#comment235405> Please do not keep old code around 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/#comment235403> Can we avoid using Proxy here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java (line 36) <https://reviews.apache.org/r/56134/#comment235406> Do we still support pooled connections? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java (line 27) <https://reviews.apache.org/r/56134/#comment235372> Please do not keep old code around. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java (line 251) <https://reviews.apache.org/r/56134/#comment235374> Are you still using this constant? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java (line 262) <https://reviews.apache.org/r/56134/#comment235375> Are you still using this constant? - Alexander Kolbasov 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 > >
