You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org> on 2018/06/13 17:07:23 UTC
Review Request 67560: SENTRY-2241: Extend the Sync Listener to pass
owner information to sentry server.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67560/
-----------------------------------------------------------
Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
Repository: sentry
Description
-------
Sentry has SentrySyncHMSNotificationsPostEventListener which is added a post listener in HMS. This listener should be extended to get the owner information of tables and databases.
Diffs
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java ccb60ff2de16a00046e317d4b48fe37e23f7337e
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java fc1c3d513f29bbc9dd2f8a3c6e1eeb4cd3e12e34
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java PRE-CREATION
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java b5e01e4c21473523b494cc624318b73ec5732408
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java 6f38ed20dcba08ec1d5a4c63fadb0611381d9c98
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java f0f08ea3d1f395ec973d735bfdb18b462f082afa
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 7f97ff7f054f797757aac94e243acdc5ee1127a2
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 52f25dc15d5b6d5e3ef1caa02baec8ed2923e290
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java d8c82970b56b1599a07f0e26edab8ed3d59b9948
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0e373a9aa5342d1b507e8b192cfdbc444242227
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java 6bfe872320d255acdb10f69e09c19623c04d8951
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java c056446262ddcf61db307eafbb785eac42973c80
Diff: https://reviews.apache.org/r/67560/diff/1/
Testing
-------
Added new tests and also made sure that existing tests passed.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 67560: SENTRY-2241: Extend the Sync Listener to
pass owner information to sentry server.
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67560/#review204935
-----------------------------------------------------------
I think we should start thinking on splitting this patch into two, one for the client and another for the server. It would be faster to review the patches separately and address any changes from the feedback.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 62 (patched)
<https://reviews.apache.org/r/67560/#comment287765>
Why was this moved here? SentryHmsEvent should be only a holder, we should keep this logic on the SentryHmsSynsPostEventListener.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 65-69 (patched)
<https://reviews.apache.org/r/67560/#comment287764>
The skipEvent should be part of the SentryHMSSyncPostEventListener logic. SentryHmsEvent is only a holder of HMS event information, so let's keep it that way.
The eventId will be -1 if there is not EVENT_ID assigned to the EventType, so we can use this -1 value later to check if an event is set or not.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 87 (patched)
<https://reviews.apache.org/r/67560/#comment287766>
Use StringUtils.equals() to compare nulls.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 106 (patched)
<https://reviews.apache.org/r/67560/#comment287767>
Should this be public?
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 110 (patched)
<https://reviews.apache.org/r/67560/#comment287768>
Should this be public?
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 116 (patched)
<https://reviews.apache.org/r/67560/#comment287769>
Could you add a comment that ownership in Hive 2.3.2 is only set for users? Better yet, put a TODO comment as a reminder when the Hive version is updated.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 145-153 (patched)
<https://reviews.apache.org/r/67560/#comment287770>
We have enough information to set the Thrift request. We should keep the SentryHmsEvent as a holder of information and build this TSentryHmsEventNotification to the Sentry client instead where all the Thrift request/response is used which IMO it is where it belongs.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 161-170 (patched)
<https://reviews.apache.org/r/67560/#comment287773>
We can use a map instead of if() sentendes.
1. It is more readable.
2. It gets the type in constant time.
See this:
private static final Map<PrincipalType, TSentryObjectOwnerType> mapOwnerType = ImmutableMap.of(
PrincipalType.ROLE, TSentryObjectOwnerType.ROLE,
PrincipalType.USER, TSentryObjectOwnerType.USER
);
private TSentryObjectOwnerType getTSentryHmsObjectOwnerType(PrincipalType principalType) {
return mapOwnerType.get(principalType);
}
If we don't want a type to be used, then we skip it in the SentryHMSPostEventListener.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 179-202 (patched)
<https://reviews.apache.org/r/67560/#comment287772>
We should put this logic back to the SentryHMSsyncPostEventListener. Let's keep this logic away of the holder object.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 218 (patched)
<https://reviews.apache.org/r/67560/#comment287763>
Gets (no Get`s).
You should explain a little more from where it gets the event ID. Mention what constant is used and what value is expected.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
Lines 223-229 (patched)
<https://reviews.apache.org/r/67560/#comment287762>
Optional:
You can write this in one line if you use the getOrDefault() method:
private long getEventId(ListenerEvent event) {
return Long.parseLong(
event.getParameters().getOrDefault(MetaStoreEventListenerConstants.DB_NOTIFICATION_EVENT_ID_KEY_NAME, "-1"));
}
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Line 117 (original), 136 (patched)
<https://reviews.apache.org/r/67560/#comment287750>
getOwner() might return NULL. You can use the StringUtils.equals(str1, str2) to compare instead.
Btw, are we going to treat 'Sergio' and 'sergio' as different users? Should we use equalsIgnoreCase() instead?
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Line 120 (original), 139 (patched)
<https://reviews.apache.org/r/67560/#comment287755>
You can check for the owner type and owner name in this if as well. You don't have to split the if() conditions, just add another || and check for the type and owner name.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Line 122 (original), 141-142 (patched)
<https://reviews.apache.org/r/67560/#comment287751>
Why was it changed from equalsIgnoreCase() to equals()? Db1 and db1 are the same for Hive.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 148 (patched)
<https://reviews.apache.org/r/67560/#comment287752>
We should not throw an exception. Why aren't we allowing HMS to change the owner and a rename at the same time? HMS has an API that allows that.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Line 24 (original), 24 (patched)
<https://reviews.apache.org/r/67560/#comment287776>
Wildcard
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1362-1366 (patched)
<https://reviews.apache.org/r/67560/#comment287778>
Should we put a comment that the privileges, including the owner privilege, are dropped by the NotificationProcessor?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1368-1372 (patched)
<https://reviews.apache.org/r/67560/#comment287779>
Actually, HMS allows users to modify any metadata value of a table, so having both owner and table renames are expected.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1430-1431 (patched)
<https://reviews.apache.org/r/67560/#comment287780>
You can use a singleton here. Sets.singleton(ownerPrivilege).
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1441-1443 (patched)
<https://reviews.apache.org/r/67560/#comment287781>
I still think this 'hive' user should not be hard-coded. What if the HMS is running as a different user?
Can we get the username from Sentry instead? We should have a way to get the user used to execute the Sentry server.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1472 (patched)
<https://reviews.apache.org/r/67560/#comment287782>
Can this method be merged with the grantOwnerPrivilege()?
Granting an owner privilege will be assigned for a single role or user, so it can have the logic to revoke any current owner from the authorizable object. Most of the code is the same in the grantOwnerPrivilege.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1377-1383 (original), 1554-1561 (patched)
<https://reviews.apache.org/r/67560/#comment287783>
You can use a map and keep the code readable. If we can do it now, we should then.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java
Lines 70 (patched)
<https://reviews.apache.org/r/67560/#comment287774>
We should keep compatibility on this API. It is public exposed, isn't it?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 288-289 (patched)
<https://reviews.apache.org/r/67560/#comment287784>
Now that this flag is used here, should we add the flag that enables the owner privilege?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 4473-4480 (original), 4613-4620 (patched)
<https://reviews.apache.org/r/67560/#comment287786>
Can this call the new method?
{
execute(Collections.singletonList(update), transactionBlock);
}
- Sergio Pena
On June 15, 2018, 8:41 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67560/
> -----------------------------------------------------------
>
> (Updated June 15, 2018, 8:41 p.m.)
>
>
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Sentry has SentrySyncHMSNotificationsPostEventListener which is added a post listener in HMS. This listener should be extended to get the owner information of tables and databases.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java PRE-CREATION
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java ccb60ff2de16a00046e317d4b48fe37e23f7337e
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java fc1c3d513f29bbc9dd2f8a3c6e1eeb4cd3e12e34
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java b5e01e4c21473523b494cc624318b73ec5732408
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java 6f38ed20dcba08ec1d5a4c63fadb0611381d9c98
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java 45dce0e7caedc755c3b1b6515830974a21c11ee5
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 5424bff67bc31d3f4ed2300531706e062f82b9ae
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 7f97ff7f054f797757aac94e243acdc5ee1127a2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 52f25dc15d5b6d5e3ef1caa02baec8ed2923e290
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java d8c82970b56b1599a07f0e26edab8ed3d59b9948
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0e373a9aa5342d1b507e8b192cfdbc444242227
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java 6bfe872320d255acdb10f69e09c19623c04d8951
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java c056446262ddcf61db307eafbb785eac42973c80
>
>
> Diff: https://reviews.apache.org/r/67560/diff/3/
>
>
> Testing
> -------
>
> Added new tests and also made sure that existing tests passed.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 67560: SENTRY-2241: Extend the Sync Listener to
pass owner information to sentry server.
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/67560/
-----------------------------------------------------------
(Updated June 19, 2018, 3:43 p.m.)
Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
Changes
-------
addressed review comments.
Repository: sentry
Description
-------
Sentry has SentrySyncHMSNotificationsPostEventListener which is added a post listener in HMS. This listener should be extended to get the owner information of tables and databases.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java PRE-CREATION
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java ccb60ff2de16a00046e317d4b48fe37e23f7337e
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java fc1c3d513f29bbc9dd2f8a3c6e1eeb4cd3e12e34
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java PRE-CREATION
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java b5e01e4c21473523b494cc624318b73ec5732408
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java 6f38ed20dcba08ec1d5a4c63fadb0611381d9c98
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java 45dce0e7caedc755c3b1b6515830974a21c11ee5
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 5424bff67bc31d3f4ed2300531706e062f82b9ae
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 7f97ff7f054f797757aac94e243acdc5ee1127a2
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 52f25dc15d5b6d5e3ef1caa02baec8ed2923e290
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java d8c82970b56b1599a07f0e26edab8ed3d59b9948
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0e373a9aa5342d1b507e8b192cfdbc444242227
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java 6bfe872320d255acdb10f69e09c19623c04d8951
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java c056446262ddcf61db307eafbb785eac42973c80
Diff: https://reviews.apache.org/r/67560/diff/4/
Changes: https://reviews.apache.org/r/67560/diff/3-4/
Testing
-------
Added new tests and also made sure that existing tests passed.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 67560: SENTRY-2241: Extend the Sync Listener to
pass owner information to sentry server.
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/67560/
-----------------------------------------------------------
(Updated June 15, 2018, 8:41 p.m.)
Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
Changes
-------
Addressed review comments from sergio
Repository: sentry
Description
-------
Sentry has SentrySyncHMSNotificationsPostEventListener which is added a post listener in HMS. This listener should be extended to get the owner information of tables and databases.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java PRE-CREATION
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java ccb60ff2de16a00046e317d4b48fe37e23f7337e
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java fc1c3d513f29bbc9dd2f8a3c6e1eeb4cd3e12e34
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java PRE-CREATION
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java b5e01e4c21473523b494cc624318b73ec5732408
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java 6f38ed20dcba08ec1d5a4c63fadb0611381d9c98
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java f0f08ea3d1f395ec973d735bfdb18b462f082afa
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 5424bff67bc31d3f4ed2300531706e062f82b9ae
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 7f97ff7f054f797757aac94e243acdc5ee1127a2
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 52f25dc15d5b6d5e3ef1caa02baec8ed2923e290
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java d8c82970b56b1599a07f0e26edab8ed3d59b9948
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0e373a9aa5342d1b507e8b192cfdbc444242227
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java 6bfe872320d255acdb10f69e09c19623c04d8951
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java c056446262ddcf61db307eafbb785eac42973c80
Diff: https://reviews.apache.org/r/67560/diff/2/
Changes: https://reviews.apache.org/r/67560/diff/1-2/
Testing
-------
Added new tests and also made sure that existing tests passed.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 67560: SENTRY-2241: Extend the Sync Listener to
pass owner information to sentry server.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 101-109 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040271#file2040271line102>
> >
> > This information is repeated in all the methods with slight changes based on the event type. Should we use a new class that builds this instead? Let's say a SentryHmsEvent class that has several constructors with different event types?
> >
> > Having a SentryHmsEvent is also better to avoid exposing the TSentryHmsEventNotification here. I should say we should expose Thrift classes only on the client class, but we can use this SentryHmsEvent as a wrapper of that class too.
I have added SentryHmsEvent which abstracts the that logic.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 177-181 (original), 263-267 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040271#file2040271line279>
> >
> > There was a good explanation of what the method does, why was it removed? We should extend the old comment to mentio what it does now.
I have made changes to this logic in the new patch. This should be addressed.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 182-184 (original), 268 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040271#file2040271line284>
> >
> > What happened with this condition?
I have made changes to this logic in the new patch. This should be addressed.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
> > Lines 339-344 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040271#file2040271line358>
> >
> > Would a map work better instead of these if conditions?
> >
> > Like:
> >
> > return mapToSentryOwnerType.get(principalType);
> >
> > We can later check in the shouldIgnoreEvent() if the type is null and just return and do not sync.
If we had to perform this in mutiple places, I agree we should consctruct a static map with this data. That is not the case. I don't see a create a mapping.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
> > Lines 282 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040274#file2040274line282>
> >
> > This class is identical to the above class (onAlterSentryRoleGrantPrivilege) except that it accept a list of TSentryPrivilege objects. We should find a way to avoid this duplication.
Assuming, you are talking about the overloaded implementation of onAlterSentryRoleRevokePrivilege API. I did not want to change the public API's.
If you don't see any issue with that I can consolidate them.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
> > Lines 409 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040274#file2040274line409>
> >
> > This class is very similar to the onAlterSentryRoleRevokePrivilege class, We should find a way to avoid this duplication.
done.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1353 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040277#file2040277line1353>
> >
> > Do we need another timer instead of grantTimer? This sync will do more things than just granting, and if the owner privilege flag is disabled, then it won't grant anything.
Added another metric.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1365-1369 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040277#file2040277line1365>
> >
> > Shouldn't the owner privilege be removed when an object is dropped?
This should be done as part of the notification handling. Currently, notificationProcessor class tries to remove privileges on DROP_DATABASE and DROP_TABLE events. That logic should be extended to handle owner privileges. We have another Jira to handle this.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1376 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040277#file2040277line1376>
> >
> > There is an extra () in the first condition.
> >
> > If the owner is empty or null, then we're not going to remove any current owner privilege from the DB? I think we should treat empty and/or null as a posible parameter to mention to remove any owner privileges.
I know we have a use case where we would want to remove the owner privilege but I prefer doing that differently.
If we use empty/null as an input to remove owner privileges we can not differentiate a legitimate case or a case where did not receive owner information by error.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1411 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040277#file2040277line1411>
> >
> > Add a comment about what this method does. Are you going to add the flag to check if the owner privilege is enabled?
Check is done in constructOwnerPrivilege.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1437-1441 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040277#file2040277line1437>
> >
> > Isn't it better to make one method call instead of if checks here? I think we can improve the method to allow empty or null privileges.
There were existing public API's which i did not want to change. we can refactor it another patch. This patch is already big and i don't to want to increase the scope of it.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1438 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040277#file2040277line1438>
> >
> > Is the grantor principal hive? We should not hardcode this value. Shouldn't we get the grantor from the caller which is HMS instead? HMS knows the user used to make this notification, doesn't it?
Typically grantorPrincipal is used to authorize if the user/group has permissions to grant an permission.
I did not find this information in the hms events. Even If we could find a way to get that information from HMS, the user/group may not have permissions to grant.
Let's take an example. user1 has create permission on the database db1 which allows him to create tables in database db1. user1 doesn't need to have grant permission.
I have hard coded "Hive" for couple of reasons
1. In case of (create database/table) user is not granting the permissions explcitly. It is the service that granting it based on the user who is creating it.
2. In case of (alter) user performing alter just needs to have alter permission on server/database to be able to do it.
3. we need to actual grantor to perform authorization, in this case we are not performing any authozation.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1453-1456 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040277#file2040277line1453>
> >
> > Why throwing an exception and logging an error? We can avoid errors here ty making clear in the javadoc of the method that only ROLE and USER are accepted parameters and other will be ignored and logged.
I agree with this comment if SentryPolicyStoreProcessor is an public API where it doesn't have a control on what is passed. In this case in particular, the requests processed by SentryPolicyStoreProcessor are sent by sentry bindings. It's reasaobale to thow exceptions when it recived content that is not expected.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1470 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040277#file2040277line1470>
> >
> > Why do we need a timer here if this method is called from sentry_notify_hms_event() which has the same timer?
You are right. It is not needed. it was stale code. I over looked it.
> On June 13, 2018, 7:51 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1505-1507 (patched)
> > <https://reviews.apache.org/r/67560/diff/1/?file=2040277#file2040277line1505>
> >
> > Shold we just log this as info and avoid throwing an exception? This is a private method and ideally it won't be called with any other type. Better yet, we should just ignore the type and not do anything.
That is my point, when it is not expected why not throw exception. If we just log and move-on we might some issues because they will not be noticed.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67560/#review204720
-----------------------------------------------------------
On June 15, 2018, 8:41 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67560/
> -----------------------------------------------------------
>
> (Updated June 15, 2018, 8:41 p.m.)
>
>
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Sentry has SentrySyncHMSNotificationsPostEventListener which is added a post listener in HMS. This listener should be extended to get the owner information of tables and databases.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java PRE-CREATION
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java ccb60ff2de16a00046e317d4b48fe37e23f7337e
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java fc1c3d513f29bbc9dd2f8a3c6e1eeb4cd3e12e34
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java b5e01e4c21473523b494cc624318b73ec5732408
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java 6f38ed20dcba08ec1d5a4c63fadb0611381d9c98
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java f0f08ea3d1f395ec973d735bfdb18b462f082afa
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 5424bff67bc31d3f4ed2300531706e062f82b9ae
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 7f97ff7f054f797757aac94e243acdc5ee1127a2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 52f25dc15d5b6d5e3ef1caa02baec8ed2923e290
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java d8c82970b56b1599a07f0e26edab8ed3d59b9948
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0e373a9aa5342d1b507e8b192cfdbc444242227
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java 6bfe872320d255acdb10f69e09c19623c04d8951
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java c056446262ddcf61db307eafbb785eac42973c80
>
>
> Diff: https://reviews.apache.org/r/67560/diff/2/
>
>
> Testing
> -------
>
> Added new tests and also made sure that existing tests passed.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 67560: SENTRY-2241: Extend the Sync Listener to
pass owner information to sentry server.
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67560/#review204720
-----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 38 (patched)
<https://reviews.apache.org/r/67560/#comment287376>
Avoid the wilcard for the imports.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Line 89 (original), 97-99 (patched)
<https://reviews.apache.org/r/67560/#comment287354>
The 'true ==' in the conditions. It is enough to say:
// is the Sentry word in the method name necessary?
if (shouldIgnoreEvent()) {
}
Should pass the EventType.CREATE_TABLE parameter only without the toString() call? We can make the private method to accept that parameter as it is required for logging only.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 101-109 (patched)
<https://reviews.apache.org/r/67560/#comment287358>
This information is repeated in all the methods with slight changes based on the event type. Should we use a new class that builds this instead? Let's say a SentryHmsEvent class that has several constructors with different event types?
Having a SentryHmsEvent is also better to avoid exposing the TSentryHmsEventNotification here. I should say we should expose Thrift classes only on the client class, but we can use this SentryHmsEvent as a wrapper of that class too.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 177-181 (original), 263-267 (patched)
<https://reviews.apache.org/r/67560/#comment287361>
There was a good explanation of what the method does, why was it removed? We should extend the old comment to mentio what it does now.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Line 182 (original), 268 (patched)
<https://reviews.apache.org/r/67560/#comment287360>
Is the notifyHmsEvent() better name?
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 182-184 (original), 268 (patched)
<https://reviews.apache.org/r/67560/#comment287362>
What happened with this condition?
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 339-344 (patched)
<https://reviews.apache.org/r/67560/#comment287367>
Would a map work better instead of these if conditions?
Like:
return mapToSentryOwnerType.get(principalType);
We can later check in the shouldIgnoreEvent() if the type is null and just return and do not sync.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 345 (patched)
<https://reviews.apache.org/r/67560/#comment287365>
This should not be an error. This parameter would be useful for skipping events, and it might be logged as info only.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 360-366 (patched)
<https://reviews.apache.org/r/67560/#comment287372>
Should the annotations docs be filled with good description of them?
API is not a subject.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 367 (patched)
<https://reviews.apache.org/r/67560/#comment287368>
Is the Sentry word needed? is it enough to say shouldIgnoreEvent()?
It should be private too.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 416 (patched)
<https://reviews.apache.org/r/67560/#comment287374>
It should be private.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
Lines 417 (patched)
<https://reviews.apache.org/r/67560/#comment287375>
What if this method is used to pass an event that does not contain the DB_NOTIFICATION_EVENT_ID_KEY_NAME? The Long.parseLong will fail.
I think we should check if that exists and return -1 if it doesn't. We should mention that in the comment as well.
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java
Lines 81-87 (patched)
<https://reviews.apache.org/r/67560/#comment287395>
You can use StringUtils.equals() from Apache commons here. It checks if the parameters are null and if not then it checks for equality on both. It is just one line of code.
Also, is 'sergio' != 'Sergio' ?
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Lines 282 (patched)
<https://reviews.apache.org/r/67560/#comment287396>
This class is identical to the above class (onAlterSentryRoleGrantPrivilege) except that it accept a list of TSentryPrivilege objects. We should find a way to avoid this duplication.
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Lines 409 (patched)
<https://reviews.apache.org/r/67560/#comment287397>
This class is very similar to the onAlterSentryRoleRevokePrivilege class, We should find a way to avoid this duplication.
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java
Lines 279 (patched)
<https://reviews.apache.org/r/67560/#comment287398>
is notifyHmsNotifications() a good name? notifySentry is redundant as the client is called SentryClient.notifySentry and we know that every method calls Sentry, right?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1353 (patched)
<https://reviews.apache.org/r/67560/#comment287400>
Do we need another timer instead of grantTimer? This sync will do more things than just granting, and if the owner privilege flag is disabled, then it won't grant anything.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1365-1369 (patched)
<https://reviews.apache.org/r/67560/#comment287401>
Shouldn't the owner privilege be removed when an object is dropped?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1376 (patched)
<https://reviews.apache.org/r/67560/#comment287402>
There is an extra () in the first condition.
If the owner is empty or null, then we're not going to remove any current owner privilege from the DB? I think we should treat empty and/or null as a posible parameter to mention to remove any owner privileges.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1387 (patched)
<https://reviews.apache.org/r/67560/#comment287403>
I don't think we should throw an exception if the HMS passed is not used by Sentry. We should log it instead as an info level that the requested event type is ignored. At the end, the request call is only to request an event. Sentry will do or will not do something with that event.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1411 (patched)
<https://reviews.apache.org/r/67560/#comment287408>
Add a comment about what this method does. Are you going to add the flag to check if the owner privilege is enabled?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1427-1428 (patched)
<https://reviews.apache.org/r/67560/#comment287404>
You can use the Collections.singleton() which returns a Set of a single object.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1437-1441 (patched)
<https://reviews.apache.org/r/67560/#comment287407>
Isn't it better to make one method call instead of if checks here? I think we can improve the method to allow empty or null privileges.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1438 (patched)
<https://reviews.apache.org/r/67560/#comment287406>
Is the grantor principal hive? We should not hardcode this value. Shouldn't we get the grantor from the caller which is HMS instead? HMS knows the user used to make this notification, doesn't it?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1453-1456 (patched)
<https://reviews.apache.org/r/67560/#comment287405>
Why throwing an exception and logging an error? We can avoid errors here ty making clear in the javadoc of the method that only ROLE and USER are accepted parameters and other will be ignored and logged.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1470 (patched)
<https://reviews.apache.org/r/67560/#comment287409>
Why do we need a timer here if this method is called from sentry_notify_hms_event() which has the same timer?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1486-1487 (patched)
<https://reviews.apache.org/r/67560/#comment287410>
You can use Collections.singleton().
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1492 (patched)
<https://reviews.apache.org/r/67560/#comment287411>
Make a comment that multiple owners are not allowed and not expected but the Sentry database does not have constraints to prevent that, so it is possible (but unlikely) that multiple owners might be returned.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1493 (patched)
<https://reviews.apache.org/r/67560/#comment287412>
Space between for(
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1505-1507 (patched)
<https://reviews.apache.org/r/67560/#comment287413>
Shold we just log this as info and avoid throwing an exception? This is a private method and ideally it won't be called with any other type. Better yet, we should just ignore the type and not do anything.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1378-1382 (original), 1553-1557 (patched)
<https://reviews.apache.org/r/67560/#comment287399>
You can put this in a map to avoid if conditions, like:
return mapSentryType.get(ownerType);
You can check for the null value returned and do something in the caller.
- Sergio Pena
On June 13, 2018, 5:07 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67560/
> -----------------------------------------------------------
>
> (Updated June 13, 2018, 5:07 p.m.)
>
>
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Sentry has SentrySyncHMSNotificationsPostEventListener which is added a post listener in HMS. This listener should be extended to get the owner information of tables and databases.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java ccb60ff2de16a00046e317d4b48fe37e23f7337e
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java fc1c3d513f29bbc9dd2f8a3c6e1eeb4cd3e12e34
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java b5e01e4c21473523b494cc624318b73ec5732408
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java 6f38ed20dcba08ec1d5a4c63fadb0611381d9c98
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java f0f08ea3d1f395ec973d735bfdb18b462f082afa
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 7f97ff7f054f797757aac94e243acdc5ee1127a2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 52f25dc15d5b6d5e3ef1caa02baec8ed2923e290
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java d8c82970b56b1599a07f0e26edab8ed3d59b9948
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0e373a9aa5342d1b507e8b192cfdbc444242227
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java 6bfe872320d255acdb10f69e09c19623c04d8951
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java c056446262ddcf61db307eafbb785eac42973c80
>
>
> Diff: https://reviews.apache.org/r/67560/diff/1/
>
>
> Testing
> -------
>
> Added new tests and also made sure that existing tests passed.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>