----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64955/#review194788 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2807 (original), 2807 (patched) <https://reviews.apache.org/r/64955/#comment273806> This in fact ends up calling getMaxPersistedIDCore(), so would it be a good opportunity to rename it now, so it would be clear we are talking about the max ID? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 148 (original), 148 (patched) <https://reviews.apache.org/r/64955/#comment273844> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 149 (original), 149 (patched) <https://reviews.apache.org/r/64955/#comment273842> lets make it clear that in fact this method returns the max ID in the store. shall we change method name to make it clear? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 205 (original), 206 (patched) <https://reviews.apache.org/r/64955/#comment273847> I thought we've agreed that at this point we shall pass to notificationFetcher.fetchNotification() notificationId-delta, where delta is configurable and probably set to some significant number, like 10. Otherwise we'll be loosing lots of older notifications with smaller IDs that got persisted since the previous fetch, due to the HMS issue. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 392 (original), 345 (patched) <https://reviews.apache.org/r/64955/#comment273803> add @param javadoc for notificationId sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 403 (original) <https://reviews.apache.org/r/64955/#comment273809> could you explain why you removed LOGGER.debug()? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Lines 48 (patched) <https://reviews.apache.org/r/64955/#comment273832> please, add javadoc for what this value stands for, since it obviously plays important role in the new logic - Vadim Spector On Jan. 4, 2018, 8:22 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64955/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2018, 8:22 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. 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 > 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/SentryService.java > 43535a7 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java > 501898b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > edde886 > > 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 > c8eef09 > > > Diff: https://reviews.apache.org/r/64955/diff/1/ > > > Testing > ------- > > Made sure that tests pass. There are three test failures which need code > change done for SENTRY-2113. > > > Thanks, > > kalyan kumar kalvagadda > >
