> On July 19, 2017, 9:27 p.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 169 (patched) > > <https://reviews.apache.org/r/60926/diff/5/?file=1780058#file1780058line173> > > > > Currently, HMSFollower looks for the latest notifications and processes > > them after persisting snapshot. > > > > You have changed it. Why dod you want to change that behavior?
To make the code cleaner. Btw, why do we have to get notifications if a snapshot is already done? Why not checking that in the next cycle? There was a comment from Sasha to call createFullSnapshot() immediatly instead of waiting for the next cycle, and I would have had to add 2 more conditions to check if createFullSnapshot() returns EMPTY or not, and continue to next notifications or not. I prefer to keep this simple buy doing one thing on each cycle. > On July 19, 2017, 9:27 p.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 193-214 (patched) > > <https://reviews.apache.org/r/60926/diff/5/?file=1780058#file1780058line197> > > > > With your change, full snapshot is taken again in below situations. > > > > 1) The current notification Id on the HMS is less than the latest > > processed by Sentry. > > 2) If the HMS and Sentry processed notifications are out-of-sync. > > > > In both these situations, I do not see a reason to retain old snapshot. > > Why don't we delete it before persisting new snapshot? That's a good question to Sasha. - Sergio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60926/#review180972 ----------------------------------------------------------- On July 19, 2017, 8:13 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60926/ > ----------------------------------------------------------- > > (Updated July 19, 2017, 8:13 p.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and > Na Li. > > > Bugs: sentry-1760 > https://issues.apache.org/jira/browse/sentry-1760 > > > Repository: sentry > > > Description > ------- > > The patch will set the 'requestHmsSnapshot' to TRUE whenever the following > cases are found: > > * List of notifications received are different than expected. > This may happen when Sentry has been down or HDFS sync was disabled for a > while (more than 24h), > and the HMS cleared old notifications (older than 24h) not processed by > Sentry causing a gap when retrieving notifications. > * Latest Sentry notification ID processed is bigger than current HMS > notification ID. > This may happen when the HMS DB data was reset or restore from an old > snapshot causing sync issues with Sentry. > > New snapshots are persisted with a new generation ID (or image number), so > there's is no need to clean-up older snapshots. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 083ee4c247f96d5c87b44b9785663a2783e6644d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java > 05518e81f52965dc1ff102dcdd446010381b9a7a > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > d67c16258c67aae997de4c0451c8b642ab05d298 > > > Diff: https://reviews.apache.org/r/60926/diff/5/ > > > Testing > ------- > > Added test cases on TestHmsFollower > > > Thanks, > > Sergio Pena > >
