----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58975/#review175154 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 316 (patched) <https://reviews.apache.org/r/58975/#comment248527> Why are we getting the value again? lastProcessedNotificationID is already passed to the method by the run() method which already read the last processed notification ID. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 316 (patched) <https://reviews.apache.org/r/58975/#comment248528> Why are we getting the value again? lastProcessedNotificationID is already passed to the method by the run() method which already read the last processed notification ID. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 429-441 (original), 428-442 (patched) <https://reviews.apache.org/r/58975/#comment248549> I feel we should wrap all this block into the processCreateDatabase() method. The same for the other events. The processCreateDatabase() could return TRUE if the event was correctly processed, FALSE otherwise. Then we could use the FALSE boolean as a condition to persist the unprocessed notification ID. This looks more readable and maintenable for future fixes. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 536 (original), 555 (patched) <https://reviews.apache.org/r/58975/#comment248546> Should we set notificationProcessed = true after this condition? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 619 (patched) <https://reviews.apache.org/r/58975/#comment248541> I think JDODataStoreException should be kept on the class which makes the JDO call to keep the abstraction of the Datanucleus calls, in this case the SentryStore class. SentryStore should throw a different Sentry exception that callers should catch. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 627 (patched) <https://reviews.apache.org/r/58975/#comment248540> Should we set notificationProcessed = true instead of persisting it? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 629 (patched) <https://reviews.apache.org/r/58975/#comment248539> Code convention: if () no if(). Also, I think we could use a better name: if (persistNotificationIdNeeded) { ... } - Sergio Pena On May 16, 2017, 5:11 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58975/ > ----------------------------------------------------------- > > (Updated May 16, 2017, 5:11 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, > Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1669 > https://issues.apache.org/jira/browse/SENTRY-1669 > > > Repository: sentry > > > Description > ------- > > Sentry should store Id's of all the notifications that it processed in a > seperate database. > HMSFollower before every run should get the last notification ID from this > persistent store. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java > 42f80aa > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java > 8d9528f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > 8fd5278 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java > f590a52 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > ef67865 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 375cf16 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java > 083e0ac > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > e7443eb > > > Diff: https://reviews.apache.org/r/58975/diff/13/ > > > Testing > ------- > > Sentry stores Id's of all the notifications that it processed in a seperate > database. > HMSFollower before every run gets the last notification ID from this > persistent store. > > > Thanks, > > kalyan kumar kalvagadda > >
