----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58975/#review175049 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java Lines 25 (patched) <https://reviews.apache.org/r/58975/#comment248365> Please use HTML formatting for ordered lists here - otherwise Javadoc looks rather ugly. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 78 (patched) <https://reviews.apache.org/r/58975/#comment248366> the variable name is now different. But more importantly, I don't think it should be a field at all since it is local and if you need it in functions you can just pass the value there when needed. I had this comment earlier and it is marked as resolved. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 96 (patched) <https://reviews.apache.org/r/58975/#comment248367> Why is it throwing exception? It just does a few assignments sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 213 (patched) <https://reviews.apache.org/r/58975/#comment248368> You do want to protect sentryStore.getLastProcessedNotificationID(); with rtry/catch, but there is no reason to protect wakeUpWaitingClientsForSync(lastProcessedNotificationID) - it may look to the reader as if both can throw an exception and then the message looks confusing, so you can move the wake up outside of try/catch block sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 216 (patched) <https://reviews.apache.org/r/58975/#comment248369> please add e here as well to know what went wrong sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 266 (original), 271 (patched) <https://reviews.apache.org/r/58975/#comment248371> Suppose that we have two HMSFollower starting up and for some reason they both decided they are masters. They both have initial value for the notification ID and both try to get full snapshot. What will prevent both of them from succeeding? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 429 (original), 434 (patched) <https://reviews.apache.org/r/58975/#comment248373> We are immediate;y catching this throw, so why do we need to do it rather then just save the ID and return? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 600 (patched) <https://reviews.apache.org/r/58975/#comment248374> Why is this the case? If we are the leader we should process the rest, but not if we are not the leader. - Alexander Kolbasov On May 15, 2017, 4:10 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58975/ > ----------------------------------------------------------- > > (Updated May 15, 2017, 4:10 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 > 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/11/ > > > 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 > >
