Re: Review Request 68968: SENTRY-2425: Add metric to track the time taken to update the owner privilege

[Arjun Mishra via Review Board]

> On Oct. 9, 2018, 6:19 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
> > Lines 124 (patched)
> > 
> >
> > Can we have one for granting and one for revoking owner privilege?
> 
> kalyan kumar kalvagadda wrote:
> Do you see a value in having seperate merics?

Is there a plan in the future to allow explicit granting and revoking of owner 
privileges? If so, its better to keep them seperated


- Arjun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68968/#review209376
---


On Oct. 9, 2018, 6:01 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68968/
> ---
> 
> (Updated Oct. 9, 2018, 6:01 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2425
> https://issues.apache.org/jira/browse/SENTRY-2425
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> New metric should be added to track the time taken by sentry server in 
> granting/updating owner privileges.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6a41ba01943bd3b2b42c9bddc9fa722f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  709434c388689b63d5db7d71cd6cc47952d647bf 
> 
> 
> Diff: https://reviews.apache.org/r/68968/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68968: SENTRY-2425: Add metric to track the time taken to update the owner privilege

[kalyan kumar kalvagadda via Review Board]

> On Oct. 9, 2018, 6:19 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
> > Lines 124 (patched)
> > 
> >
> > Can we have one for granting and one for revoking owner privilege?

Do you see a value in having seperate merics?


- kalyan kumar


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68968/#review209376
---


On Oct. 9, 2018, 6:01 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68968/
> ---
> 
> (Updated Oct. 9, 2018, 6:01 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2425
> https://issues.apache.org/jira/browse/SENTRY-2425
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> New metric should be added to track the time taken by sentry server in 
> granting/updating owner privileges.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6a41ba01943bd3b2b42c9bddc9fa722f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  709434c388689b63d5db7d71cd6cc47952d647bf 
> 
> 
> Diff: https://reviews.apache.org/r/68968/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68968: SENTRY-2425: Add metric to track the time taken to update the owner privilege

[Arjun Mishra via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68968/#review209376
---




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
Lines 124 (patched)


Can we have one for granting and one for revoking owner privilege?


- Arjun Mishra


On Oct. 9, 2018, 6:01 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68968/
> ---
> 
> (Updated Oct. 9, 2018, 6:01 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2425
> https://issues.apache.org/jira/browse/SENTRY-2425
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> New metric should be added to track the time taken by sentry server in 
> granting/updating owner privileges.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6a41ba01943bd3b2b42c9bddc9fa722f 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  709434c388689b63d5db7d71cd6cc47952d647bf 
> 
> 
> Diff: https://reviews.apache.org/r/68968/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Review Request 68968: SENTRY-2425: Add metric to track the time taken to update the owner privilege

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68968/
---

Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.


Bugs: SENTRY-2425
https://issues.apache.org/jira/browse/SENTRY-2425


Repository: sentry


Description
---

New metric should be added to track the time taken by sentry server in 
granting/updating owner privileges.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
 d34340cd6a41ba01943bd3b2b42c9bddc9fa722f 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 709434c388689b63d5db7d71cd6cc47952d647bf 


Diff: https://reviews.apache.org/r/68968/diff/1/


Testing
---


Thanks,

kalyan kumar kalvagadda

Re: Review Request 68890: SENTRY-2300: Move Permission Update due to DDL to HMS Post Event Listener

[Na Li via Review Board]

> 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 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

Re: Review Request 68890: SENTRY-2300: Move Permission Update due to DDL to HMS Post Event Listener

[Na Li via Review Board]

> 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)
> > 
> >
> > 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

[Na Li via Review Board]

> 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 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 
> 

Re: Review Request 68918: SENTRY-2423: Increase the allocation size for auto-increment of id's for Snapshot tables.

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68918/#review209369
---


Ship it!




Ship It!

- Na Li


On Oct. 3, 2018, 8:24 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68918/
> ---
> 
> (Updated Oct. 3, 2018, 8:24 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2423
> https://issues.apache.org/jira/browse/SENTRY-2423
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry uses native strategy for auto generation of ID columns for 
> which the allocation can not be increased.
> This should be change to "increment" strategy and which lets to configure the 
> allocation size. This reduces the delay caused for checking the 
> SEQUENCE_TABLE.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  e90fe2df46266d72f1eb3706b948fb163a44c4a1 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  6539e33e021e0f5acaa7827356a8e9b3e4286b18 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1722109e381bc7be81a4673d12c1ac9f81195c47 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  a299e008cc0df92f3d692024c7794aa494b64d2d 
> 
> 
> Diff: https://reviews.apache.org/r/68918/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure all the existing tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68890: SENTRY-2300: Move Permission Update due to DDL to HMS Post Event Listener

[kalyan kumar kalvagadda via Review Board]

---
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)


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)


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 68918: SENTRY-2423: Increase the allocation size for auto-increment of id's for Snapshot tables.

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68918/#review209363
---


Ship it!




Looks good

- Sergio Pena


On Oct. 3, 2018, 8:24 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68918/
> ---
> 
> (Updated Oct. 3, 2018, 8:24 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2423
> https://issues.apache.org/jira/browse/SENTRY-2423
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry uses native strategy for auto generation of ID columns for 
> which the allocation can not be increased.
> This should be change to "increment" strategy and which lets to configure the 
> allocation size. This reduces the delay caused for checking the 
> SEQUENCE_TABLE.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  e90fe2df46266d72f1eb3706b948fb163a44c4a1 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  6539e33e021e0f5acaa7827356a8e9b3e4286b18 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1722109e381bc7be81a4673d12c1ac9f81195c47 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  a299e008cc0df92f3d692024c7794aa494b64d2d 
> 
> 
> Diff: https://reviews.apache.org/r/68918/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure all the existing tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

[Arjun Mishra via Review Board]

> On Oct. 9, 2018, 1:29 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
> > Line 422 (original), 422 (patched)
> > 
> >
> > Use authorization objects which is how Sentry defines the HMS objects, 
> > isn't it?

Works.


> On Oct. 9, 2018, 1:29 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3437 (patched)
> > 
> >
> > I think we should use time (like every 5min) instead of using a 
> > threshold. We don't know if persisting is in a lock situation or it is just 
> > too slow, so waiting until the # of objects has reached a specific number 
> > could be slower too.
> > 
> > Btw, I don't think this time should be configurable. Once a snapshot is 
> > happning we cannot change the configuration, and once the snapshot is done 
> > we don't need to change it either. We have too many configurations already.

Will make it by time instead and hard code it to 5 mins


> On Oct. 9, 2018, 1:29 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3439 (patched)
> > 
> >
> > What about printing the numbers of objects persisted as well? That 
> > number + the time would give us an idea of how much time is taken to 
> > persist an X number of objects.
> > 
> > For instance:
> > - 5:00am HMS snapshot persisting progress: authz_objs_persisted=100 
> > authz_paths_persisted=500 authz_objs_size=1020 authz_paths_size=5500
> > - 5:05am HMS snapshot persisting progress: authz_objs=403 
> > authz_paths=1020 authz_objs_size=1020 authz_paths_size=5500
> > 
> > You can deduct from the above that 303 authorization objects and 620 
> > paths are persisted every 5min, so there is another 10m to finish.
> > 
> > Btw, we should specify that this is an HMS snapshot. Just using 'number 
> > of objects' does not help users to understand what objects are being 
> > persisted.

Got it


- Arjun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68958/#review209360
---


On Oct. 8, 2018, 10:16 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68958/
> ---
> 
> (Updated Oct. 8, 2018, 10:16 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2419
> https://issues.apache.org/jira/browse/SENTRY-2419
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Process of persisting the snapshot might take longer based on the snapshot 
> size. It would be helpfull to to be able to understand where the sentry 
> sentry stands in the process of persisting.
> 
>  
> 
> Adding a Metric for this will be helpfull in debugging.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  e90fe2df4 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  b6dca7aa8 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1722109e3 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  3a68eb6bf 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  059621a68 
> 
> 
> Diff: https://reviews.apache.org/r/68958/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68958: SENTRY-2419: Log where sentry stands in the process of persisting the snpashot

[Arjun Mishra via Review Board]

> On Oct. 9, 2018, 1:29 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
> > Lines 180-183 (patched)
> > 
> >
> > Why do we need these metrics? Isn't having the progress printed in the 
> > log enough? These are also printed in the log so we are incrementing the 
> > log size with duplicated messages.
> > 
> > Btw, where are these metrics updated? Also, what if there is another 
> > snapshot, would these metrics contains only the snapshot committed or 
> > persisted?

Metrics will continue to be logged after the snapshot is done. If this doesn't 
affect performance can we still keep it?


- Arjun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68958/#review209360
---


On Oct. 8, 2018, 10:16 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68958/
> ---
> 
> (Updated Oct. 8, 2018, 10:16 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2419
> https://issues.apache.org/jira/browse/SENTRY-2419
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Process of persisting the snapshot might take longer based on the snapshot 
> size. It would be helpfull to to be able to understand where the sentry 
> sentry stands in the process of persisting.
> 
>  
> 
> Adding a Metric for this will be helpfull in debugging.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  e90fe2df4 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  d34340cd6 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java
>  b6dca7aa8 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1722109e3 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  3a68eb6bf 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  059621a68 
> 
> 
> Diff: https://reviews.apache.org/r/68958/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>