> On Oct. 9, 2018, 2:13 p.m., kalyan kumar kalvagadda wrote: > > 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.
Kalyan, for your concerns 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.", I copy my response in Jira sentry-2300 here [Lina] This is not true. With current approach, if processing the notification event fails, the event ID is saved and no longer processed again and you made that code change avoid getting stuck at illegal event (what you stated above is that the event will be refectched and processed again), same as the new approach I am taking. see https://github.com/apache/sentry/blob/master/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java#L493 public void processNotifications(Collection<NotificationEvent> events) throws Exception { boolean isNotificationProcessed; if (events.isEmpty()) { return; } for (NotificationEvent event : events) { isNotificationProcessed = false; try { // Only the leader should process the notifications if (!isLeader()) { LOGGER.debug("Not processing notifications since not a leader"); return; } isNotificationProcessed = notificationProcessor.processNotificationEvent(event); } catch (Exception e) { if (e.getCause() instanceof JDODataStoreException) { LOGGER.info("Received JDO Storage Exception, Could be because of processing " + "duplicate notification"); if (event.getEventId() <= sentryStore.getLastProcessedNotificationID()) { // Rest of the notifications need not be processed. LOGGER.error("Received event with Id: {} which is smaller then the ID " + "persisted in store", event.getEventId()); break; } } else { LOGGER.error("Processing the notification with ID:{} failed with exception {}", event.getEventId(), e); } } if (!isNotificationProcessed) { try { // Update the notification ID in the persistent store even when the notification is <-- if the processing of event fails, the notification ID is still saved and not fetched again // not processed as the content in in the notification is not valid. // Continue processing the next notification. LOGGER.debug("Explicitly Persisting Notification ID = {} ", event.getEventId()); sentryStore.persistLastProcessedNotificationID(event.getEventId()); } catch (Exception failure) { LOGGER.error("Received exception while persisting the notification ID = {}", event.getEventId()); throw failure; } } // Wake up any HMS waiters that are waiting for this ID. wakeUpWaitingClientsForSync(event.getEventId()); } } - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68890/#review209365 ----------------------------------------------------------- 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 > >
