> 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?
synching in HMSFollower meaning synchornizing with HMS. Defination of being synchronized with HMS differs when HDFS sync feature is enabled OR disable. > 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? 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. > 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 235 (patched) > > <https://reviews.apache.org/r/65533/diff/1/?file=1953486#file1953486line235> > > > > Doesn't this required notificationId = ... like the previous > > isResyncRequired call? nope. in previous case, notificationId is used to fetch the notifications but in this case control will would return. In the subsequent run the notificationId is fetched from the database again. > 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? 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. > On Feb. 7, 2018, 10:14 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java > > Line 62 (original), 62 (patched) > > <https://reviews.apache.org/r/65533/diff/1/?file=1953488#file1953488line62> > > > > Avoid using the * when importing packages. will change. > On Feb. 7, 2018, 10:14 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java > > Lines 354 (patched) > > <https://reviews.apache.org/r/65533/diff/1/?file=1953488#file1953488line357> > > > > 'i stead' should be 'instead' will change. > On Feb. 7, 2018, 10:14 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java > > Lines 355 (patched) > > <https://reviews.apache.org/r/65533/diff/1/?file=1953488#file1953488line358> > > > > What will it prevent? will update. > On Feb. 7, 2018, 10:14 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java > > Lines 486-488 (patched) > > <https://reviews.apache.org/r/65533/diff/1/?file=1953488#file1953488line489> > > > > I see this block repeated several times, can we create a method for it? will change - kalyan kumar ----------------------------------------------------------- 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 > >
