----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61973/#review184215 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 4135 (patched) <https://reviews.apache.org/r/61973/#comment260300> We can also set pm.setDetachAllOnCommit(false) and another setting that avoids loading data on close. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 4136 (patched) <https://reviews.apache.org/r/61973/#comment260299> hash is a primary key, so we can set unique. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 63 (original), 64 (patched) <https://reviews.apache.org/r/61973/#comment260288> This should be HiveCOnnectionFactory as well sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 247 (original), 251 (patched) <https://reviews.apache.org/r/61973/#comment260289> Style - is it preferrable to use // style comments to simplify commenting out block of code with /* comments? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 248 (original), 252 (patched) <https://reviews.apache.org/r/61973/#comment260290> wording: number of notifications can't have agap. sequence of notifications can. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 254 (original), 258 (patched) <https://reviews.apache.org/r/61973/#comment260291> Wording here. May be you can reword it here. E.g. HMS notifications may contain both gaps in the sequence and duplicates (the same ID repeated more then once for different events). Duplicates do not necessarily come one after another, so the sequence {1,2,3,2} is possible sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 260 (original) <https://reviews.apache.org/r/61973/#comment260292> So you decided not to log anything in this case? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Lines 41 (patched) <https://reviews.apache.org/r/61973/#comment260295> can be final sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Lines 43 (patched) <https://reviews.apache.org/r/61973/#comment260296> can be final sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Lines 47 (patched) <https://reviews.apache.org/r/61973/#comment260297> can use HashSet<> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Lines 49 (patched) <https://reviews.apache.org/r/61973/#comment260298> this and all other methods can be package-private sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Lines 127 (patched) <https://reviews.apache.org/r/61973/#comment260315> If I am reading this code correctly, the following will happen: 1) Every time we request IDs starting from last seen ID, so normally the first ID in the sequence is always a duplicate 2) This first ID in the sequence isn't in the cache yet, so we go to the sentryStore and fetch the hash and compare. This means that we always go to sentryStore for the frist element of the batch which means every time we call fetchNotifications(). This is not very bad, but if we store the sha1 hash of the last event in the cache we don't have to go to sentryStore at all under normal circumstances - Alexander Kolbasov On Aug. 29, 2017, 6:41 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61973/ > ----------------------------------------------------------- > > (Updated Aug. 29, 2017, 6:41 p.m.) > > > Review request for sentry, Alexander Kolbasov and Na Li. > > > Bugs: sentry-1888 > https://issues.apache.org/jira/browse/sentry-1888 > > > Repository: sentry > > > Description > ------- > > Approach: > The proposed solution will be to request notifications from the last ID seen > again. This way we could bring current duplicates and apply them on Sentry. > We have the risk to miss duplicates that were committed much time later, but > we cannot trust on those duplicates as they will not know the order of the > time they were committed. > > The patch adds a new helper class called HiveNotificationFetcher that allow > us to bring new notifications and apply a filter to those notifications that > were already processed by Sentry. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UniquePathsUpdate.java > 7dae2f538602f4c264084791fb45bb6891a34941 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 593b92f96b47f959266280bce3d0809f6a80c362 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 35da6fc7908ec7498d1fd658d75b62028df35751 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java > 4a8fb953ce486e1aeb1042884de56872b5539cd0 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > f56384a99e128b3e9880cbc5692107d61f2f500f > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcher.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java > 77a2bbb214e23ca146c2934ee4d3bf15e592906f > > > Diff: https://reviews.apache.org/r/61973/diff/1/ > > > Testing > ------- > > > Thanks, > > Sergio Pena > >
