----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60926/#review180972 -----------------------------------------------------------
Sergio, I might have some more comments. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 169 (patched) <https://reviews.apache.org/r/60926/#comment256354> 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? 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/#comment256365> 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? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 206-214 (patched) <https://reviews.apache.org/r/60926/#comment256366> we have same check in getNotifications method in SentryHMSClient class. We need to remove one of them. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java Lines 69 (patched) <https://reviews.apache.org/r/60926/#comment256362> method name testPersistAFullSnapshotWhenNoSnapshotISProcessedYet is better. - kalyan kumar kalvagadda 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 > >
