----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64820/#review194489 -----------------------------------------------------------
Seems like I have no problems with the code once my comments in the previous review are addressed. Would like to add more checks to the tests, including, say, a couple of loop iterations for the state after "sentry-ahead" state has been cleared, just to make sure HMSFollower is in the normal flow mode (no Sentry-ahead condition, gets partial updates normally, does not attempt full updates for no good reason, hmsFollower.getNotificationSyncRetryCount() keeps returning 0). sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 263 (original), 275 (patched) <https://reviews.apache.org/r/64820/#comment273272> typo "cuurent" sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java Lines 1108 (patched) <https://reviews.apache.org/r/64820/#comment273275> Would it make sense to add verify() on sentryHmsClient.getFullSnapshot() invokactions? Can't hurt it seems Also, would it make sense to continue the loop for sometime after count == interruptCount instead of break? To check the normal flow after "sentry-ahead" recovery? And add default "else" clause with verify()'s for all mock objects? Post-sentry-ahead state is an important part to test too. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java Lines 1179 (patched) <https://reviews.apache.org/r/64820/#comment273274> Would it make sense to add verify() on sentryHmsClient.getFullSnapshot() invocations? Can't hurt it seems. Also, would it make sense to continue the loop for sometime after count == interruptCount instead of break? To check the normal flow after "sentry-ahead" recovery? And add default "else" clause with verify()'s for all mock objects? Post-sentry-ahead state is an important part to test too. - Vadim Spector On Dec. 24, 2017, 1:50 a.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64820/ > ----------------------------------------------------------- > > (Updated Dec. 24, 2017, 1:50 a.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/2/ > > > Testing > ------- > > mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestHMSFollower > > > Thanks, > > Arjun Mishra > >
