> On Jan. 25, 2018, 11:42 p.m., Vadim Spector wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > > Line 235 (original), 270 (patched) > > <https://reviews.apache.org/r/64955/diff/4-5/?file=1943745#file1943745line272> > > > > It seems better to produce two dufferent logs, e.g. > > > > if(isFoundInCache(notificationEvent.getEventId(), hash) == true) { > > LOGGER.debug("Ignoring HMS notification already processed: ID = {}", > > notificationEvent.getEventId()); > > return false; > > } else if (sentryStore.isNotificationProcessed(hash) > > LOGGER.debug("Ignoring HMS notification already processed: ID = {} - > > cache miss", notificationEvent.getEventId()); > > return false; > > } > > > > Note "cache miss" in the second log. > > > > Cache miss is what we try to avoid for better performance. It may > > happen sometimes, but if there are too many "cache miss" messages, it's > > useful to know.
As discussed, also suggesting adding unit test to make sure if incoming IDs have no out-of-order or duplicate elements (though can still have gaps), then in theory there should be no cache misses in LRUMap, i.e. sentryStore.isNotificationProcessed(hash) is not invoked. It's to insure that if HMS folks fix the problems on their side with the IDs, this code will perform no unnecessary DB lookups. - Vadim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64955/#review196297 ----------------------------------------------------------- On Jan. 25, 2018, 5 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64955/ > ----------------------------------------------------------- > > (Updated Jan. 25, 2018, 5 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/HMSFollower.java > 2f2b98412e7dfdcc847ffe7975a70f452554e747 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > edea5b64d8f98c93aafc1fe43fa97e00c2ce2948 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > 93cc34f34c044dceb11d27e41ecbd1a14f64bed9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 96c6810baa4d554db2b7d3739a28e3ff7e8b33a0 > > 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/TestHMSFollower.java > 79030780c35e5bda432e3ec3f01328e627cb50a6 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java > 91c90f9d302f4feb3a8b3d06541f43541c87bf0f > > 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/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/5/ > > > Testing > ------- > > Made sure that tests pass. > > > Thanks, > > kalyan kumar kalvagadda > >
