----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65533/#review197810 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java Lines 125-126 (patched) <https://reviews.apache.org/r/65533/#comment278088> Make sure to add an statement about what users should do in case this scenario happens. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java Lines 223 (patched) <https://reviews.apache.org/r/65533/#comment278089> Can we avoid this extra call by setting the notificationId to 0? The setLastProcessedNotificatioNID already persists the ID to 0, so I don't see a necessary getLastProcessedNotificatoiNID call. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java Line 261 (original), 279 (patched) <https://reviews.apache.org/r/65533/#comment278090> isAuthzPathsSnapshotEmpty() checks if MAuthzPathsSnapshotId is empty, why did you change it to MAuthzPathsMapping? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java Lines 307 (patched) <https://reviews.apache.org/r/65533/#comment278091> Exception if an error occurs attempting to fetch the latest notification ID from the HMS sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 587-589 (patched) <https://reviews.apache.org/r/65533/#comment278092> What about MAuthzPathsSnapshotId.class? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 3115 (original), 3131 (patched) <https://reviews.apache.org/r/65533/#comment278093> The name of the method and what is checks does not match. Why is it needed to check if the MAuthzPathsMapping is empty instead of the MAuthzPathsSnapshotId? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java Lines 520-522 (patched) <https://reviews.apache.org/r/65533/#comment278094> Should you delete these comments? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java Lines 584-587 (patched) <https://reviews.apache.org/r/65533/#comment278095> Should you delete these comments? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java Lines 1148-1152 (patched) <https://reviews.apache.org/r/65533/#comment278097> What's wrong with /* */? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java Lines 997-1000 (original), 1159-1162 (patched) <https://reviews.apache.org/r/65533/#comment278096> Should you delete these comments? - Sergio Pena On Feb. 15, 2018, 9 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65533/ > ----------------------------------------------------------- > > (Updated Feb. 15, 2018, 9 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-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > eae7861728f2bc11b4c1b44aa3b61b881a87740b > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > cf764eda1a006ce96f301e3ecb87749e05ba4a09 > > 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/NotificationProcessor.java > e7558370025c6acd83492615be093f2bd16a202b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 96c6810baa4d554db2b7d3739a28e3ff7e8b33a0 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java > 79030780c35e5bda432e3ec3f01328e627cb50a6 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > 4cd00e6672730773c74b9840247d1f4d5f7bdfe4 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java > e64f5cd687bf59133d6475c912ebdd7930601151 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java > 91397061e78a6524410d35643dd3a33be8353ecc > > > Diff: https://reviews.apache.org/r/65533/diff/3/ > > > Testing > ------- > > Made sure that all the tests passed. > > > Thanks, > > kalyan kumar kalvagadda > >
