You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Na Li via Review Board <no...@reviews.apache.org> on 2018/10/01 20:19:36 UTC
Review Request 68890: SENTRY-2300: Move Permission Update due to DDL
to HMS Post Event Listener
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68890/
-----------------------------------------------------------
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-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/TestNotificationProcessor.java f227bb4
Diff: https://reviews.apache.org/r/68890/diff/1/
Testing
-------
Add new test cases in TestSentryPolicyStoreProcessor, and update tests in TestHMSFollower and TestNotificationProcessor. Tests in TestSentryPolicyStoreProcessor, TestHMSFollower and TestNotificationProcessor passed.
Thanks,
Na Li
Re: Review Request 68890: SENTRY-2300: Move Permission Update due to
DDL to HMS Post Event Listener
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> 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.
>
> Na Li wrote:
> 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());
> }
> }
Kalyan,
for your concern 2) "We need to unerstand one thing. Root cause for this delay in processing the notifications."
One major reason is getting full snapshot or notification event takes too long. HMS has to wait for sentry to get notification events from HMS. It causes nesty dependency circle. It takes at least 0-500 ms delay for HMS to get back from waiting for sentry getting notification asynchronously. You can ask Arjun to confirm that.
- 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
>
>
Re: Review Request 68890: SENTRY-2300: Move Permission Update due to
DDL to HMS Post Event Listener
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> 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
>
>
Re: Review Request 68890: SENTRY-2300: Move Permission Update due to
DDL to HMS Post Event Listener
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 68890: SENTRY-2300: Move Permission Update due to
DDL to HMS Post Event Listener
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Oct. 4, 2018, 6:26 p.m., Arjun Mishra wrote:
> > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
> > Line 36 (original), 36 (patched)
> > <https://reviews.apache.org/r/68890/diff/2/?file=2093692#file2093692line36>
> >
> > Can you revert changes to this file? Nothing has changed here?
It is auto-generated by thrift.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68890/#review209228
-----------------------------------------------------------
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
>
>
Re: Review Request 68890: SENTRY-2300: Move Permission Update due to
DDL to HMS Post Event Listener
Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68890/#review209228
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
Line 36 (original), 36 (patched)
<https://reviews.apache.org/r/68890/#comment293547>
Can you revert changes to this file? Nothing has changed here?
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java
Line 308 (original), 308 (patched)
<https://reviews.apache.org/r/68890/#comment293545>
We can delete this method since it is not going to be invoked anywhere?
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
Line 1171 (original), 1171 (patched)
<https://reviews.apache.org/r/68890/#comment293543>
Can we delete this method since its not being invoked anywhere anymore?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Line 1707 (original), 1883 (patched)
<https://reviews.apache.org/r/68890/#comment293546>
Can we remove this method since not being invoked?
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
Lines 93 (patched)
<https://reviews.apache.org/r/68890/#comment293548>
Isn't the default value on create false? We should have a test with default behavior
- Arjun Mishra
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
>
>
Re: Review Request 68890: SENTRY-2300: Move Permission Update due to
DDL to HMS Post Event Listener
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/68890/diff/1-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