> On Dec. 28, 2017, 5:21 p.m., kalyan kumar kalvagadda wrote: > > I have couple of comments on the approach taken. Before going into that, I > > would re-iterate couple of assumptions that HMSFollower has taken > > > > 1. Method getCurrentNotificationEventId on Hmsclient would always return > > the last issued notification event id, which HMSFollower assumes to be the > > biggest. Which doesn't seems to be true. We don't have any conclusive > > answers on this behavior of HMS. > > 2. First event in list of notifications received from HMS greater than > > latest sentry HMS notification Id. This is not true > > Event_ID in the NOTIFICATION_LOG table are not always ascending. Order of > > sequence is not guaranteed. > > There can be gaps in between Event_ID's in the consecutive entries. > > Here is what I think. > > > > when getCurrentNotificationEventId is not dependable why should HMSFollower > > use it in the first place. > > Even with the fix that you provided HMSFollower would wait for 5 > > sec(Default) to see if getCurrentNotificationEventId returns a value > > greater than the last event id that sentry has processed. If not, full > > snapshot is initiated. There is no guaranty that it would be greater by > > that time. Sentry is not out of sync even if getCurrentNotificationEventId > > returns an Id that is smaller than the last event id that sentry has > > processed. Do you agree? > > > > I prefer removing code in HMSFollower that depends on > > getCurrentNotificationEventId to see if it out of sync with HMS. > > Fix for SENTRY-2109 will address the 2nd assumption taken.
I agree about removing dependency on getCurrentNotificationEventId, but I don't see another way to test for cases where Sentry is "ahead". Unless you want to remove the case to trigger a full snapshot if Sentry is "ahead"? The motivation for this patch was to KEEP THE EXISTING logic but make it more restrictive. The fix should have an improvement because in 5 sec, or 10 retires, Sentry should have been "ahead" on EVERY retry for a Full Snapshot to occur. But if all agree that Sentry being "ahead" should not be a criteria to trigger a ful snapshot we can remove it. - Arjun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64820/#review194585 ----------------------------------------------------------- On Dec. 25, 2017, 10:49 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64820/ > ----------------------------------------------------------- > > (Updated Dec. 25, 2017, 10:49 p.m.) > > > Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar > kalvagadda, Na Li, Sergio Pena, and Vadim Spector. > > > Repository: sentry > > > Description > ------- > > Once SentryHMSNotification table is not empty, there are two cases when a > full snapshot is triggered. > > If first event in list of notifications received from HMS greater than latest > sentry hms notification Id > If latest sentry notification Id is greater than current hms notification Id > The later is a unexpected case where for some reason sentry gets ahead of > HMS. We should add a logic to trigger a full snapshot in case 2 only after a > configurable number of retries. This will avoid unnecessary full snapshot > triggers as they are resource intensive > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > a9afb151a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > aa1b6a31c > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > edde886a7 > > > Diff: https://reviews.apache.org/r/64820/diff/3/ > > > Testing > ------- > > mvn -f sentry-provider/sentry-provider-db/pom.xml test > > > Thanks, > > Arjun Mishra > >
