----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65533/#review196971 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java Lines 125 (patched) <https://reviews.apache.org/r/65533/#comment276976> Should you update the following condition for DBUpdateForwarder.getAllUpdatesFrom()? Replace if (seqNum > curSeqNum) { // No new notifications were processed. return Collections.emptyList(); } With if (seqNum > curSeqNum) { if(seqNum = curSeqNum + 1) { // No new notifications were processed. return Collections.emptyList(); } else { // perm change has been reset, get full perm snapshot List<K>fullImage = retrieveFullImage(); //Only log if we have received full image if( !fullImage.isEmpty()) { LOGGER.info("({}) A newer full update with image number {} was found and being sent to HDFS", retrieverType, curImgNum); } return fullImage; } } Once you clear the perm change, the seqNum from Name Node will be larger than curSeqNum. The correct behavior should be to send full snapshot of permission to Name Node after you clear the perm changes. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 596 (patched) <https://reviews.apache.org/r/65533/#comment276977> This method is not just for test, called in HMSFollower line 122. Why do you have this comment "the method only for test"? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 615 (patched) <https://reviews.apache.org/r/65533/#comment276978> it is called in HMSFollower, why comments "the method only for test"? - Na Li 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 > >
