> On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote: > > sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java > > Lines 90 (patched) > > <https://reviews.apache.org/r/60883/diff/5/?file=1780704#file1780704line90> > > > > This file has only cosmetic changes - do we really need these changes? > > Less files to touch.
It is not just cosmetic changes. It makes sure the default values for AUTHZ_SERVER_NAME and AUTHZ_SERVER_NAME_DEPRECATED are the same > On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote: > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java > > Line 54 (original), 54 (patched) > > <https://reviews.apache.org/r/60883/diff/5/?file=1780705#file1780705line54> > > > > Why this change? The original comment was reasonable and matches the > > code. no need to change this. I will revert it > On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 49 (patched) > > <https://reviews.apache.org/r/60883/diff/5/?file=1780706#file1780706line52> > > > > This field isn't really used for anything, just for tests which can > > easily extract the value from NotificationProcessor. will change > On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Line 76 (original), 76 (patched) > > <https://reviews.apache.org/r/60883/diff/5/?file=1780706#file1780706line79> > > > > Should it really be public? Yes. TestHMSFollowerSentryStoreIntegration is under org.apache.sentry.provider.db.service.persistent (so I don't need to change SentryStore functions to public), and HMSFollower is under org.apache.sentry.service.thrift > On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Line 304 (original), 312 (patched) > > <https://reviews.apache.org/r/60883/diff/5/?file=1780706#file1780706line315> > > > > Why is this public? save reason above. It is made to be public for testing purpose. Brian asks me to remove @VisibleForTesting since it is public. > On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java > > Line 59 (original), 59 (patched) > > <https://reviews.apache.org/r/60883/diff/5/?file=1780707#file1780707line59> > > > > Why this change? Ommitting public works fine. If I remove "public", I will get this error "/home/lina/sw/sentry/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java:39: error: HiveSimpleConnectionFactory is not public in org.apache.sentry.service.thrift; cannot be accessed from outside package" > On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java > > Lines 19 (patched) > > <https://reviews.apache.org/r/60883/diff/5/?file=1780708#file1780708line19> > > > > Why this test lives here rather then in the same package as HMSFollower? It tests integration of HMSFollower and SentryStore. If I put it under the same package as HMSFollower, I need to change a lot of functions in SentryStore to be public to setup the tests. > On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > > Lines 213 (patched) > > <https://reviews.apache.org/r/60883/diff/5/?file=1780709#file1780709line213> > > > > Do you need @Test here? > > > > Please document what this test case is testing Yes. Will add > On July 21, 2017, 3:27 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > > Lines 223 (patched) > > <https://reviews.apache.org/r/60883/diff/5/?file=1780709#file1780709line223> > > > > Please document what this case is testing. Same for tests below. will do - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60883/#review181118 ----------------------------------------------------------- On July 21, 2017, 2:30 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60883/ > ----------------------------------------------------------- > > (Updated July 21, 2017, 2:30 p.m.) > > > Review request for sentry, kalyan kumar kalvagadda, Sergio Pena, and Vamsee > Yarlagadda. > > > Repository: sentry > > > Description > ------- > > There are several problems: > 1) HMSFollower should read sentry-site.xml to get the authen server name, not > from hive-site.xml through hiveConf. The input configuration for HMSFollower > constructor is from sentry-site.xm. We should use that configuration to get > server name. > 2) There are two variable names that could hold the value of the server. > "hive.sentry.server"is deprecated and is in > HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME_DEPRECATED. The new name is > sentry.hive.server in HiveAuthzConf.AuthzConfVars.AUTHZ_SERVER_NAME. We > should check the sentry.hive.server first. If it is not set, check the > deprecated hive.sentry.server to be backward compatible. If it is still not > set, use default value > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java > e1312bf > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java > dccbbb6 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 547a61f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java > 3d67401 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > fdf52bf > > > Diff: https://reviews.apache.org/r/60883/diff/5/ > > > Testing > ------- > > create integration tests. reproduced the issue and verfied the fix > > > Thanks, > > Na Li > >
