----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68890/#review209365 -----------------------------------------------------------
Lina, I see advantages of this approach but i also see some dis-advantages which are serious. 1. If there is a failure while handling the notification for any reason there is no way to retry. This is not the case if we process the event from notification log as HMSfollower would not update the notification ID in the database unless it is process. This will force HMSFollower to fetch it again and process it. 2. We know that customers are observing issues with sync listener and are used to disable the sync listerner when timeut exceptions are observed. We need to unerstand one thing. Root cause for this delay in processing the notifications. This delay could also be because of delay in updating the permissions as well. Unless we know why are the notifications processed slowly, moving the logic of updating the sentry permissions synconously would effect a lot of customer as their jobs gets slowdown. That was the case before. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1659-1695 (patched) <https://reviews.apache.org/r/68890/#comment293752> Functionally dropSentryDbPrivileges and alterSentryTablePrivileges are same. There is no need to have two methods. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1698-1783 (patched) <https://reviews.apache.org/r/68890/#comment293753> Is this code directly copied from NotificationProcessor? Are there any other chnages to this part of the code. - kalyan kumar kalvagadda On Oct. 2, 2018, 9:26 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68890/ > ----------------------------------------------------------- > > (Updated Oct. 2, 2018, 9:26 p.m.) > > > Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio > Pena. > > > Bugs: sentry-2300 > https://issues.apache.org/jira/browse/sentry-2300 > > > Repository: sentry > > > Description > ------- > > Move the privilege updates from Notification event processing at > NotificationProcessor to HMS post event processing at > SentryPolicyStoreProcessor > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java > e0f35e8 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java > c37f370 > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java > 98de8e3 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java > 6eb8521 > > sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java > 7cdf148 > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java > 5fc299b > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 68d864c > > sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift > 3364648 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > 709434c > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java > 7b7d0e1 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java > 059621a > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java > 0d62941 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java > a886836 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestNotificationProcessor.java > f227bb4 > > > Diff: https://reviews.apache.org/r/68890/diff/2/ > > > Testing > ------- > > Add new test cases in TestSentryPolicyStoreProcessor, and update tests in > TestHMSFollower and TestNotificationProcessor. Tests in > TestSentryPolicyStoreProcessor, TestHMSFollower and TestNotificationProcessor > passed. > > > Thanks, > > Na Li > >
