----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56134/#review164166 -----------------------------------------------------------
sentry-core/sentry-core-common/pom.xml (lines 117 - 122) <https://reviews.apache.org/r/56134/#comment235754> Do we need this? I don't see us overriding the default configuration so we probably might not need it? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (lines 49 - 71) <https://reviews.apache.org/r/56134/#comment235755> Spacing is off across all files. https://wiki.cloudera.com/display/engineering/Java+Style+Guide 2 space for all new lines. 4 space for all continuation lines. Should be an easy fix once you set up your IDE configuration. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (lines 67 - 116) <https://reviews.apache.org/r/56134/#comment235778> Is this an inner class? Can't we have this outside? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 83) <https://reviews.apache.org/r/56134/#comment235855> I must have missed this but do we ever say UserGroupInformation.loginFromKeytab? Otherwise we always pick the kerberos from the local context and not necessarily from the keytab that was passed to the client application? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (lines 122 - 125) <https://reviews.apache.org/r/56134/#comment235789> Don't have a strong opinion about this, but I would prefer to see these inline at the usages itself as these are not anyway preserved outside of this constructor. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 153) <https://reviews.apache.org/r/56134/#comment235793> for (HostAndPort hostPort : hostsAndPorts) ? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 160) <https://reviews.apache.org/r/56134/#comment235794> Can we add javadocs for these functions to better understand the params needed? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 174) <https://reviews.apache.org/r/56134/#comment235796> For consistency, this.serverAddress sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 199) <https://reviews.apache.org/r/56134/#comment235801> If we don't initialize SentryServiceClientTransportDefaultImpl via SentryServiceClientTransportDefaultImpl(Configuration conf, ServiceTransportConstants.sentryService type) then connectionFullRetryTotal is always set to 0 and thus we will never get a connection setup done? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 230) <https://reviews.apache.org/r/56134/#comment235841> For consistency, can we make this method "synchronized" as well? It is any reentrant as long as it is the same thread which is accessing both connect and connectWithRetry? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (lines 231 - 232) <https://reviews.apache.org/r/56134/#comment235838> Put this under else section for better readability? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 259) <https://reviews.apache.org/r/56134/#comment235840> Does referring to "transport" prints simply an object reference or some detailed explanation? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryServiceClientTransportDefaultImpl.java (line 263) <https://reviews.apache.org/r/56134/#comment235821> During my testing for HMSFollower, I noticed that transport always returns isOpen as "true" even if the other end dies. But when we try to access the transport for messages, it then throws "Broken pipe" error. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/HdfsServiceTransportConstants.java (lines 33 - 39) <https://reviews.apache.org/r/56134/#comment235761> This looks like a bit out of place. Based on the rest of the class, doing this map initialization here doesn't sound right to me. Why don't we expose the SASL default values as constants and let the caller populate this map? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/HdfsServiceTransportConstants.java (lines 43 - 45) <https://reviews.apache.org/r/56134/#comment235759> Which parameter are we referring to? SECURITY_MODE? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/HdfsServiceTransportConstants.java (lines 46 - 70) <https://reviews.apache.org/r/56134/#comment235758> Why can't these constants be exposed as "public"? It looks like that's how even Hadoop implements it so that the references would be a lot easier. http://github.mtv.cloudera.com/CDH/hadoop/blob/cdh5-2.6.0/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/PolicyServiceTransportConstants.java (lines 59 - 70) <https://reviews.apache.org/r/56134/#comment235764> Same comment as my other comments on exposing these using helper methods. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (lines 59 - 60) <https://reviews.apache.org/r/56134/#comment235835> We should rather be using ServiceConstants.ServerConfig kerberos constants to keep in sync rather than duplicating constants. This applies to all the other places where we refer to kerberos constants. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ServiceTransportConstants.java (lines 62 - 84) <https://reviews.apache.org/r/56134/#comment235763> Yeah I agree about the comment made by Sasha. I don't think we should be exposing constants using helper methods. Example hadoop configs: http://github.mtv.cloudera.com/CDH/hadoop/blob/cdh5-2.6.0/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java (line 34) <https://reviews.apache.org/r/56134/#comment235765> Remove this? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java (lines 40 - 45) <https://reviews.apache.org/r/56134/#comment235772> Reason for commenting this out. sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/SentryHdfsServiceIntegrationBase.java (line 58) <https://reviews.apache.org/r/56134/#comment235766> We shouldn't instantiate a constant class but rather directly refer to the static final variables of the class. sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/SentryHdfsServiceIntegrationBase.java (lines 59 - 62) <https://reviews.apache.org/r/56134/#comment235847> How come these two are just the same? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java (line 196) <https://reviews.apache.org/r/56134/#comment235768> Remove? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java (lines 486 - 493) <https://reviews.apache.org/r/56134/#comment235769> If it is no longer needed, can we remove it? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java (lines 18 - 33) <https://reviews.apache.org/r/56134/#comment235770> Please cleanup the sections that are no longer needed. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientFactory.java (lines 58 - 62) <https://reviews.apache.org/r/56134/#comment235771> Reason for commenting this out. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1957) <https://reviews.apache.org/r/56134/#comment235850> typo? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java (line 27) <https://reviews.apache.org/r/56134/#comment235851> Remove this. sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.derby.sql (line 15) <https://reviews.apache.org/r/56134/#comment235852> Can you explain the motivation behind changing this value? - Vamsee Yarlagadda 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 > >
