> On Feb. 7, 2018, 10:14 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java > > Line 205 (original), 212-213 (patched) > > <https://reviews.apache.org/r/65533/diff/1/?file=1953486#file1953486line212> > > > > Isn't the new method name more confusing than before? > > isFullSnapshotRequired() means Sentry requires a full HMS snapshot while > > isReSyncRequired() sounds like Sentry requires to sync with HDFS, but the > > hdfsSyncEnable after this contradicts the name, isn't it? > > kalyan kumar kalvagadda wrote: > synching in HMSFollower meaning synchornizing with HMS. Defination of > being synchronized with HMS differs when HDFS sync feature is enabled OR > disable.
I think we should keep the isFullSnapshotRequired() name as the meaning makes more sense for what it does 'see if a full HMS snapshot is required'. isReSyncRequired() could conflict with HDFS sync meaning which they're different. > On Feb. 7, 2018, 10:14 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java > > Lines 217-220 (patched) > > <https://reviews.apache.org/r/65533/diff/1/?file=1953486#file1953486line217> > > > > This needs a better explanation. I'm trying to understand the reason of > > these lines. A re-sync with HDFS needed? But HDFS is disabled, right? > > kalyan kumar kalvagadda wrote: > re-synching in HMSFollower meaning synchornizing with HMS. > Let's hdfs sync is disabled and sentry detects that it is out of sync, it > should set the last processed eventid as 0 and process all the notification > event in HMS NOTIFICATION_LOG table. I see that there is an if (hdfsSyncEnabled) inside the createFullSnapshot() method. Can we put this block inside the createFullSnapshot() method instead? > On Feb. 7, 2018, 10:14 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java > > Line 254 (original), 274 (patched) > > <https://reviews.apache.org/r/65533/diff/1/?file=1953486#file1953486line274> > > > > Why do we need a method change? Is this resync meant for hdfs or hms > > full snapshot? > > kalyan kumar kalvagadda wrote: > Idea of this patch is to avaoid getting full snapshot when hdfs sync is > not enabled. > > method isFullSnapshotRequired, tells us if sentry is out-of-sync with > HMS. > > isFullSnapshotRequired is not correct. Please suggest if > "isReSyncRequired" is not reasonable. isFullSnapshopt() is more reasonable. isReSyncRequired() conflicts with areNotificationOutOfSync() and Hdfs Sync. - Sergio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65533/#review197051 ----------------------------------------------------------- On Feb. 6, 2018, 7:06 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65533/ > ----------------------------------------------------------- > > (Updated Feb. 6, 2018, 7:06 p.m.) > > > Review request for sentry, Alexander Kolbasov, Na Li, and Sergio Pena. > > > Bugs: SENTRY-2115 > https://issues.apache.org/jira/browse/SENTRY-2115 > > > Repository: sentry > > > Description > ------- > > **Scenario-1:** When HDFS sync is disabled, and sentry is started for the > first time. > **Scenario-2:** When HDFS sync is disabled, and current event-id from HMS is > less than last event-d processed by sentry > **Scenario-3:** When HDFS sync is disabled, and first event-id in the > subsequent pull is not greater than the last event-id processed by sentry by > 1. > **New Behavior:** Full snapshots need not be taken in all > When Sentry detects out-of-sync situations, it should reset > SENTRY_HMS_NOTIFICATION_ID table and start processing the event in > HMS_NOTIFICATION_LOG from beginning. > > **Scenario-4:** Initially HDFS sync was enabled and later disabled for while > and then HDFS sync is enabled and sentry service is restarted to get it to > effect. > **New Behavior:** When Sentry detects out-of-sync situations, it should reset > SENTRY_HMS_NOTIFICATION_ID table and start processing the event in > HMS_NOTIFICATION_LOG from beginning. > To handle scenario explained in Scenario-4, sentry should reset the mapping > information when ever HDFS sync is disabled. That way it can learn from > scratch when the feature is enabled back. There is no value is holding stale > data even when we know it will have issues when the feature is enabled back. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java > 2f2b98412e7dfdcc847ffe7975a70f452554e747 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java > 79030780c35e5bda432e3ec3f01328e627cb50a6 > > > Diff: https://reviews.apache.org/r/65533/diff/1/ > > > Testing > ------- > > Made sure that all the tests passed. > > > Thanks, > > kalyan kumar kalvagadda > >
