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