> On May 16, 2017, 12:12 a.m., Alexander Kolbasov wrote: > > 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/diff/11/?file=1718410#file1718410line25> > > > > Please use HTML formatting for ordered lists here - otherwise Javadoc > > looks rather ugly.
Will Change the format. > On May 16, 2017, 12:12 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 78 (patched) > > <https://reviews.apache.org/r/58975/diff/11/?file=1718416#file1718416line79> > > > > 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. Will fix. > On May 16, 2017, 12:12 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 96 (patched) > > <https://reviews.apache.org/r/58975/diff/11/?file=1718416#file1718416line98> > > > > Why is it throwing exception? It just does a few assignments Constructors previously used to call getLastProcessedNotificationID() which can throw exception. Now it is not case. I will fix it. > On May 16, 2017, 12:12 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 213 (patched) > > <https://reviews.apache.org/r/58975/diff/11/?file=1718416#file1718416line221> > > > > 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 will fix it. > On May 16, 2017, 12:12 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 216 (patched) > > <https://reviews.apache.org/r/58975/diff/11/?file=1718416#file1718416line224> > > > > please add e here as well to know what went wrong Will fix. > On May 16, 2017, 12:12 a.m., Alexander Kolbasov wrote: > > 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/diff/11/?file=1718416#file1718416line280> > > > > 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? These changes do not handle that scenario. I have some idea on how to do it. I will raise seperate jira for that. Changes made are intended to make sure that sentry records the ID's of all the notificaitons that it receives from HMS in the persistant store. I prefer to limit the scope of this jira. > On May 16, 2017, 12:12 a.m., Alexander Kolbasov wrote: > > 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/diff/11/?file=1718416#file1718416line453> > > > > We are immediate;y catching this throw, so why do we need to do it > > rather then just save the ID and return? I agree, that is what i commented when lina was working in part of the logic. As this was not in the scope of this jira, i did not make it. I can surely add it. > On May 16, 2017, 12:12 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 600 (patched) > > <https://reviews.apache.org/r/58975/diff/11/?file=1718416#file1718416line620> > > > > Why is this the case? If we are the leader we should process the rest, > > but not if we are not the leader. we would end up in this situation when multiple sentry servers are acting as leader. event.getEventId() <= sentryStore.getLastProcessedNotificationID() indicates that HMSFollower in other sentry server is ahead of it self. There is no point is processing subsequest notifications so this logic skips the processing of rest of the notifications. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58975/#review175049 ----------------------------------------------------------- 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 > >
