----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64820/#review194474 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 219 (patched) <https://reviews.apache.org/r/64820/#comment273241> isSentryAhead() is part of the logic to decide if full snapshot is required, so it would make sense to invoke it inside the isFullSnapshotRequired(), instead of adding this block of code here. After all, isSentryAhead() is an improved version of the code that used to be exactly in isFullSnapshotRequired(), which you've removed. And the javadoc section for isFullSnapshotRequired() that you removed would stay, just with some minor modifications. It will result in fewer diffs and will be easier to undrestand what's exactly changed. - Vadim Spector On Dec. 22, 2017, 11:34 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64820/ > ----------------------------------------------------------- > > (Updated Dec. 22, 2017, 11:34 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/1/ > > > Testing > ------- > > mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestHMSFollower > > > Thanks, > > Arjun Mishra > >
