----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64820/#review194585 -----------------------------------------------------------
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. - kalyan kumar kalvagadda 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 > >
