> On May 24, 2017, 4:03 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Line 261 (original) > > <https://reviews.apache.org/r/59508/diff/1/?file=1730900#file1730900line262> > > > > Why do you remove this log message? It is useful to know the info
getMetaStoreClient, already has similar log on success. Removed it as they are redundent. > On May 24, 2017, 4:03 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Line 335 (original), 179 (patched) > > <https://reviews.apache.org/r/59508/diff/1/?file=1730900#file1730900line336> > > > > since you make "lastProcessedNotificationID" local instead of the class > > member, when full snapshot is not obtained in this run(), "if > > (eventId.getEventId() > lastProcessedNotificationID)" will be true even > > though some event are obtained before. It will result in getting the same > > events for many times. This logic is not right, and have performance issue. I did not understand your comment. What is the case where same event is processed multiple times and what isthe performace issue you see here? > On May 24, 2017, 4:03 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java > > Lines 664 (patched) > > <https://reviews.apache.org/r/59508/diff/1/?file=1730902#file1730902line693> > > > > This is bug that Sasha has fixed. It should be > > > > return "true" > > .equalsIgnoreCase((authzConf.get(syncConfVar.getVar(), > > syncConfVar.getDefault()))); If it was fixed, it should have been in the code base. I just moved the code around. > On May 24, 2017, 4:03 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java > > Lines 232 (patched) > > <https://reviews.apache.org/r/59508/diff/1/?file=1730903#file1730903line233> > > > > could dbName be null? or SentryStore.isNull(dbName)? Yes, it could be. This already checked in line 231. !SentryStore.isNULL(authzble.getDb() > On May 24, 2017, 4:03 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > > Line 57 (original), 57 (patched) > > <https://reviews.apache.org/r/59508/diff/1/?file=1730905#file1730905line57> > > > > should you test NotificationProcessor in TestNotificationProcessor.java? These tests in HMSFollower don't test the NotificationProcessor class. These tests are making sure that appropriate notification function is called. That is it.There should be seperate test class for testing the implmentation of NotificationProcessor. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59508/#review175941 ----------------------------------------------------------- On May 23, 2017, 11:23 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59508/ > ----------------------------------------------------------- > > (Updated May 23, 2017, 11:23 p.m.) > > > Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, > Sergio Pena, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1769 > https://issues.apache.org/jira/browse/SENTRY-1769 > > > Repository: sentry > > > Description > ------- > > Things included in refactoring. > 1. Moved the complete notification processing logic to notificationProcessor. > 2. Added new class HMSFollowerHelper class which does > HMS Client creation > HMS Client closure > Creating and persisting the HMS snapshot > 3. Misc cleanup > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > 0bd0833 > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestKrbConnectionTimeout.java > b62a83f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java > 0d54548 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 8450402 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 58e8881 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerHelper.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java > de8e2f7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java > 215f7d5 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java > dd37e7e > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > 74a5afb > > > Diff: https://reviews.apache.org/r/59508/diff/1/ > > > Testing > ------- > > Made sure that all the tests around HMSFollower and SentryStore passed. > > > Thanks, > > kalyan kumar kalvagadda > >
