> 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. > > Arjun Mishra wrote: > 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.
Firstly, Sentry can never be ahead of HMS. That's not a possibility under normal circumstances. I think, we can remove that condition which triggers full snapshot. HMSFollower is thinking that it is, because of it's assumtion that value returned by getCurrentNotificationEventId is always greater than the MAX(event-id) that sentry processed. This situation is possible only when HMS data is reset and all the data is reset. we need to understad the possibilities of this. When some thing like this happens, administrator should request for snapshot explicitly. There is seperate effort being put on this. - kalyan kumar ----------------------------------------------------------- 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 > >
