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/01/04 21:35:11 UTC
Review Request 64960: SENTRY-2113: MSentryHmsNotification should also
hold the notification hash.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64960/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
Bugs: SENTRY-2113
https://issues.apache.org/jira/browse/SENTRY-2113
Repository: sentry
Description
-------
Here is the description of code changes done
1. Made code changes to sql scripts to have notificaiton has in SENTRY_HMS_NOTIFICATION_ID table with unique index.
2. Made JDO changes to have notification has in MSentryHmsNotification
3. Added additional logging which will be helpfull in debugging.
4. Code changes to persist notification has along with event-id in SENTRY_HMS_NOTIFICATION_ID table.
5. Lookup MSentryHmsNotification to know if the notification is processed instead of using MSentryPathChange.
6. Avoid looking for MSentryHmsNotification to descide if snapshot is needed. As this is not correct. If there are no notifications after a snapshot is taken there will be full snpashots taken again and again. Instead i changes it to use Path Mapping information.
7. Fixed the tests
Diffs
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UniquePathsUpdate.java 38b4e2a
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 34180e7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo d883c51
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java 849e5f8
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 6c4631f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 097aa62
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java d09da5f
sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.derby.sql 477d224
sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.mysql.sql 33e36c7
sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.oracle.sql db66b13
sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.postgres.sql 2746922
sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-2.0.0.sql 69d8a24
sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-2.0.0.sql 6c55d28
sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.0.0.sql df34f87
sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-2.0.0.sql 6526ab2
sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-2.0.0.sql 25f3356
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.8.0-to-2.0.0.sql cf5a450
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b410027
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java 964a56c
Diff: https://reviews.apache.org/r/64960/diff/1/
Testing
-------
Made sure all the tests are passing.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 64960: SENTRY-2113: MSentryHmsNotification should
also hold the notification hash.
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/64960/
-----------------------------------------------------------
(Updated Jan. 8, 2018, 3:26 p.m.)
Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
Changes
-------
Addressed comments from Lina.
Bugs: SENTRY-2113
https://issues.apache.org/jira/browse/SENTRY-2113
Repository: sentry
Description
-------
Here is the description of code changes done
1. Made code changes to sql scripts to have notificaiton has in SENTRY_HMS_NOTIFICATION_ID table with unique index.
2. Made JDO changes to have notification has in MSentryHmsNotification
3. Added additional logging which will be helpfull in debugging.
4. Code changes to persist notification has along with event-id in SENTRY_HMS_NOTIFICATION_ID table.
5. Lookup MSentryHmsNotification to know if the notification is processed instead of using MSentryPathChange.
6. Avoid looking for MSentryHmsNotification to descide if snapshot is needed. As this is not correct. If there are no notifications after a snapshot is taken there will be full snpashots taken again and again. Instead it is changes to use Path Mapping information.
7. Fixed the tests
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UniquePathsUpdate.java 38b4e2a
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 34180e7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo d883c51
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java 849e5f8
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 6c4631f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java f1a5b10
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentrySchemaTool.java d75e24b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 097aa62
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java d09da5f
sentry-provider/sentry-provider-db/src/main/resources/010-SENTRY-2113.derby.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/010-SENTRY-2113.mysql.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/010-SENTRY-2113.oracle.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/010-SENTRY-2113.postgres.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-2.1.0.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-2.1.0.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.1.0.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-2.1.0.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-2.1.0.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-2.0.0-to-2.1.0.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-derby-2.0.0-to-2.1.0.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-mysql-2.0.0-to-2.1.0.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-oracle-2.0.0-to-2.1.0.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-postgres-2.0.0-to-2.1.0.sql PRE-CREATION
sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.db2 5ae9349
sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.derby 770e1b5
sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.mysql 770e1b5
sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.oracle 770e1b5
sentry-provider/sentry-provider-db/src/main/resources/upgrade.order.postgres 770e1b5
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b410027
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java 964a56c
Diff: https://reviews.apache.org/r/64960/diff/2/
Changes: https://reviews.apache.org/r/64960/diff/1-2/
Testing
-------
Made sure all the tests are passing.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 64960: SENTRY-2113: MSentryHmsNotification should
also hold the notification hash.
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/64960/#review194896
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java
Line 77 (original), 103 (patched)
<https://reviews.apache.org/r/64960/#comment273971>
When compare, we should check notificationId first and then notificationHash to minimize the overhead when two istances are different.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 2762 (original), 2763 (patched)
<https://reviews.apache.org/r/64960/#comment273972>
need to add comment for event
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2779 (patched)
<https://reviews.apache.org/r/64960/#comment273973>
In another place, you use "String eventHash = ((UniquePathsUpdate) update).getEventHash();" for event hash. Can you take the same approach to be consistent?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2781 (patched)
<https://reviews.apache.org/r/64960/#comment273974>
ID is not provided for logging
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3838 (patched)
<https://reviews.apache.org/r/64960/#comment273975>
should you check if eventHash is null?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3842 (original), 3855 (patched)
<https://reviews.apache.org/r/64960/#comment273976>
do you need to check
if(notificationId <= 0 || eventHash == null || eventHash.isEmpty())
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 4230 (original), 4244 (patched)
<https://reviews.apache.org/r/64960/#comment273977>
should we use "equals(String)" to compare if two strings are of the same value. This is recommanded in http://www.datanucleus.org/products/accessplatform_4_0/jdo/jdoql.html
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Line 250 (original), 251 (patched)
<https://reviews.apache.org/r/64960/#comment273978>
If HDFS is disabled, will MAuthzPathsMapping cotain anything? If not, you need to add a check that if HDFS is disabled, return false before this check. Otherwise, when HDFS is disabled, sentry will always get full snapshot.
- Na Li
On Jan. 4, 2018, 9:35 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64960/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2018, 9:35 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio Pena, and Vadim Spector.
>
>
> Bugs: SENTRY-2113
> https://issues.apache.org/jira/browse/SENTRY-2113
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Here is the description of code changes done
> 1. Made code changes to sql scripts to have notificaiton has in SENTRY_HMS_NOTIFICATION_ID table with unique index.
> 2. Made JDO changes to have notification has in MSentryHmsNotification
> 3. Added additional logging which will be helpfull in debugging.
> 4. Code changes to persist notification has along with event-id in SENTRY_HMS_NOTIFICATION_ID table.
> 5. Lookup MSentryHmsNotification to know if the notification is processed instead of using MSentryPathChange.
> 6. Avoid looking for MSentryHmsNotification to descide if snapshot is needed. As this is not correct. If there are no notifications after a snapshot is taken there will be full snpashots taken again and again. Instead it is changes to use Path Mapping information.
> 7. Fixed the tests
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UniquePathsUpdate.java 38b4e2a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java 34180e7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo d883c51
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java 849e5f8
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 6c4631f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java aa1b6a3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java 097aa62
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java d09da5f
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.derby.sql 477d224
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.mysql.sql 33e36c7
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.oracle.sql db66b13
> sentry-provider/sentry-provider-db/src/main/resources/008-SENTRY-1569.postgres.sql 2746922
> sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-2.0.0.sql 69d8a24
> sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-2.0.0.sql 6c55d28
> sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-2.0.0.sql df34f87
> sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-2.0.0.sql 6526ab2
> sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-2.0.0.sql 25f3356
> sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.8.0-to-2.0.0.sql cf5a450
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b410027
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java edde886
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java 964a56c
>
>
> Diff: https://reviews.apache.org/r/64960/diff/1/
>
>
> Testing
> -------
>
> Made sure all the tests are passing.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>