> On May 10, 2017, 10:29 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java > > Lines 91 (patched) > > <https://reviews.apache.org/r/58975/diff/6/?file=1714466#file1714466line91> > > > > Where is the seqNum value set?
seqNum is the eventID in the notification which is passed to processNotificationEvents while processing the notification. This seqNumber will them be part of PathsUpdate created. > On May 10, 2017, 10:29 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 3413 (patched) > > <https://reviews.apache.org/r/58975/diff/6/?file=1714467#file1714467line3413> > > > > Would it actually return null in this case? Yes, I have tested it. > On May 10, 2017, 10:29 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Line 95 (original), 100 (patched) > > <https://reviews.apache.org/r/58975/diff/6/?file=1714468#file1714468line101> > > > > Why are you doing this in constructor? We need to make these decisions > > anyway when we call run() needHiveSnapshot is actually set in the constructor so I had to keep that logic there. If i have to move the initialization of currentEventID to run method i have to move initialization of needHiveSnapshot also. I'm fine with that. > On May 10, 2017, 10:29 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Line 267 (original), 282 (patched) > > <https://reviews.apache.org/r/58975/diff/6/?file=1714468#file1714468line283> > > > > This should be assigned from sentrystore like we do everywhere else - > > assigning this from event seems wrong Currently notification id is persisted in two places. 1. when PATH_UPDATE is persisted to DB 2. When HMSFollower gets SentryInvalidHMSEventException while processing notification. When a snapshot is taken, there would be no entries in the notification table. eventIDBefore has the notification ID of the last notification in notification log. we should infact persist the notification ID in to store here. I will make that change as well. > On May 10, 2017, 10:29 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 285 (patched) > > <https://reviews.apache.org/r/58975/diff/6/?file=1714468#file1714468line286> > > > > We don't know that, but we know that something went wrong. We do neeed > > to read it every time because we do not keep in-memory state currentNotificationID is initialized from SentryStore on every run. I have updated the comment. > On May 10, 2017, 10:29 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 555 (patched) > > <https://reviews.apache.org/r/58975/diff/6/?file=1714468#file1714468line567> > > > > Can we handle this as a separate Catch statement instead? Done > On May 10, 2017, 10:29 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 571 (patched) > > <https://reviews.apache.org/r/58975/diff/6/?file=1714468#file1714468line583> > > > > In all other places we assign it from SentrySTore but here suddenly we > > assign it from the eventId which looks wrong. when currentNotificationID is initialized it is done based on the value from sentry store. Later when HMSFollower processes the notifications, currentNotificationID is update based on the eventId in the notification that is processed. Just to avoid unnecessary DB operation. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58975/#review174579 ----------------------------------------------------------- On May 11, 2017, 12:55 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58975/ > ----------------------------------------------------------- > > (Updated May 11, 2017, 12:55 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 > e271ea4 > > 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/7/ > > > 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 > >
