----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58975/#review174868 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java Lines 24 (patched) <https://reviews.apache.org/r/58975/#comment248094> I don't think this comment belongs there, but please put a comment explaining that we are using key constraint to prevent concurrent updates to the same value. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 80 (patched) <https://reviews.apache.org/r/58975/#comment248083> I don't think this should be a class-level variable - it is always used as a local. Also, this is called currentNotificationID and there is also CurrentNotificationEventId which looks very similar and this confuses me every time. Can you come up with some other name which looks distinct? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 96 (original), 103 (patched) <https://reviews.apache.org/r/58975/#comment248086> We shouldn't be doing this in constructor, but in run() method instead. For example, sentryStore.getLastProcessedNotificationID() may throw an exception and you wouldn't even construct HMSFollower which is wrong. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 120 (patched) <https://reviews.apache.org/r/58975/#comment248085> Why are we doing this in constructor? This seems wrong - we should do it in the run() method. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 303 (original), 320 (patched) <https://reviews.apache.org/r/58975/#comment248087> This is confusing - you are supposed to get it from sentryStore but here you get the value from the event. Please use different variables to hold the two - otherwise it isn't clear what value are you actually using sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 326 (patched) <https://reviews.apache.org/r/58975/#comment248089> why having else case here? We can do it unconditionally sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 329 (patched) <https://reviews.apache.org/r/58975/#comment248088> We only need to do the wakeup if we processed the full snapshot - that was the logic before. Otherwise we did it at the beginning of run() sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 322 (original), 343 (patched) <https://reviews.apache.org/r/58975/#comment248090> This line is way too long sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 604 (patched) <https://reviews.apache.org/r/58975/#comment248092> Is it always JDODataStoreException or we can get different exceptions fro DataNucleus here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 607 (patched) <https://reviews.apache.org/r/58975/#comment248093> should it be < or <= ? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java Lines 2471 (patched) <https://reviews.apache.org/r/58975/#comment248095> I don't see how it can be null - Alexander Kolbasov On May 12, 2017, 5:38 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58975/ > ----------------------------------------------------------- > > (Updated May 12, 2017, 5:38 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/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 > 5e6b906 > > 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/10/ > > > 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 > >
