----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64820/#review194472 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 71 (patched) <https://reviews.apache.org/r/64820/#comment273240> A couple of naming suggestions: a) fullUdateRetryCount is a bit misleading; count is something that's incremented, not a constant. fullUpdateAttemptMax would be more intuitive, makes it clearly related to fullUpdateAttemptCount. Or you can use "Retry" in both names - fullUpdateRetryCount and fullUpdateRetryMax. b) the names are probably misleading altogether; it is not an attempt to do full update, but on contrary an attempt to avoid it; essentially, an attempt to establish that Sentry and HMS are in-sync, so no full update is needed. I'd change it to something like notificationSyncRetryCount and notificationSyncRetryMax sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 228 (patched) <https://reviews.apache.org/r/64820/#comment273239> nit: extra line - 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 > >
