> On May 16, 2017, 9:23 p.m., Sergio Pena wrote: > > 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/diff/12/?file=1721685#file1721685line446> > > > > 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.
You are taking about refactoring of the current code. Why don't we handle as seperate jira. I will create one. I don't want to delay committing this functionality on this. > On May 16, 2017, 9:23 p.m., Sergio Pena wrote: > > 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/diff/12/?file=1721685#file1721685line573> > > > > Should we set notificationProcessed = true after this condition? For some reason, the current code calls processAddPartition after printing the error. If processAddPartition is called the notificatfion ID will be persisted. Could be an issue. will fix it > On May 16, 2017, 9:23 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 627 (patched) > > <https://reviews.apache.org/r/58975/diff/12/?file=1721685#file1721685line646> > > > > Should we set notificationProcessed = true instead of persisting it? Wounldn't it be confusing? - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58975/#review175154 ----------------------------------------------------------- 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 > >
