----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64955/#review195758 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Lines 66 (patched) <https://reviews.apache.org/r/64955/#comment275054> why not "private final LRUMap cache;"? The code never resets the reference. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Lines 75 (patched) <https://reviews.apache.org/r/64955/#comment275053> Please, add javadoc here: what does x3 factor mean? What's the assumption here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Lines 112 (patched) <https://reviews.apache.org/r/64955/#comment275052> Nit: just "return cache.get(hash) != null;" sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Line 89 (original), 147 (patched) <https://reviews.apache.org/r/64955/#comment275056> You create filter for the minFetchId which you immediately update on the next line. One-line comment on what's going on here would not hurt. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Line 92 (original), 154 (patched) <https://reviews.apache.org/r/64955/#comment275058> Let's log both lastEventId and minFetchEventId. So we'd be clear on what's the biggest ID we received and what's the starting ID we are fetching now. Something like: LOGGER.debug("Requesting HMS notifications since ID = {}, last ID {}", minFetchId, lastEventId); This way when we try to make sense out of the logs, we don't need to do math each time. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Line 96 (original), 158 (patched) <https://reviews.apache.org/r/64955/#comment275061> It's a brand-new logic here, critical for valdiating this whole new design. Can you add javadoc summarizing: why does HiveNotificationFetcher decide here that it is out of sync with HMS? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Lines 205 (patched) <https://reviews.apache.org/r/64955/#comment275062> this section of code suggests that foundLastProcessedNotification can only change from false to true. Can it change from true to false? If yes, where can it happen? If not, why? Please, add comment on that. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Line 206 (original) <https://reviews.apache.org/r/64955/#comment275057> why did you delete the cache.clear() line? - Vadim Spector On Jan. 17, 2018, 11:56 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64955/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2018, 11:56 p.m.) > > > Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio > Pena, and Vadim Spector. > > > Bugs: SENTRY-2109 > https://issues.apache.org/jira/browse/SENTRY-2109 > > > Repository: sentry > > > Description > ------- > > This patch does couple of things > 1. Avoid triggering full snapshots when gaps are observed while fetching new > notifications. While fetching new notifications HMSFollower would would fetch > notifications with last event-id as well. When it gets the notifications and > if it doesn't get the notifications with event-id, full snpshot is triggered. > 2. Functinalty to address gaps and out-of-sequence notificaitons by > re-fetching addtional notifications that were already fetched. This solution > is not fool proof. It does a best effort to reduse the chance of loosing > notifications by re-fetching the notifications.This approach will introduce > an over head of fetching addtional notifications that were already fetched. > Overhead of DB look-up is addressed by using a cache. This reduces additional > DB lookups needed to check if the notification was already processed. > 2. Added looging to report duplicate events and out of order events for debug > purpose. > 3. Added new e2e tests to verfy this behavior. > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryOutOfSyncException.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 6c4631fa74760d8721b5740dd3dffb2c1d8e72e6 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > aa1b6a31c28f35af86952c213d5e20a8c9bb3490 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > 097aa62912e92ece7f8da0ac0fccb124579a88f2 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 43535a7b50fea51049f3142837736e6a99a3a80f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 7e02874b4be6a7109108809b1f404fe971b7b8e2 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java > 501898bca261db2daf937a8d803d12a59616192b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > b4100278392986c161625a366212c6fef66ec0a9 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > edde886a7646539499149f2d86758979436567bd > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHiveNotificationFetcher.java > 83a1becd46ac2d69c7d6dd05ed6253d1cdd8800d > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestSnapshotCreation.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestSnapshotCreationWithShorterHMSEventTtl.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestSnapshotWithLongerHMSFollowerLongerInterval.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > 4cd00e6672730773c74b9840247d1f4d5f7bdfe4 > > > Diff: https://reviews.apache.org/r/64955/diff/3/ > > > Testing > ------- > > Made sure that tests pass. > > > Thanks, > > kalyan kumar kalvagadda > >
