> On Jan. 8, 2018, 4:35 p.m., Arjun Mishra wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 357 (patched) > > <https://reviews.apache.org/r/64955/diff/2/?file=1931085#file1931085line406> > > > > Can we make this WARN?
Unless, it is creating an issue I don't see a reason to log in WARNING. WARNING level is used to WARN the admin looking at it so that something is wrong. When we know that this is expected with the current behaviour of HMS we don't have to log it as warning. > On Jan. 8, 2018, 4:35 p.m., Arjun Mishra wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 359 (patched) > > <https://reviews.apache.org/r/64955/diff/2/?file=1931085#file1931085line408> > > > > Can we make this WARN? Same as above. > On Jan. 8, 2018, 4:35 p.m., Arjun Mishra wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > > Lines 98 (patched) > > <https://reviews.apache.org/r/64955/diff/2/?file=1931086#file1931086line98> > > > > Should foundLastProcessedNotification=false be initialized before the > > if block Nope. Do you have a reason why it should be initialized before if block? > On Jan. 8, 2018, 4:35 p.m., Arjun Mishra wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > > Lines 109 (patched) > > <https://reviews.apache.org/r/64955/diff/2/?file=1931086#file1931086line109> > > > > The log message should say "Event-Id - 1 of the last event processed by > > sentry..." lastEventId is the max event ID processed my sentry it is not Event-Id - 1. > On Jan. 8, 2018, 4:35 p.m., Arjun Mishra wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > > Line 137 (original), 164 (patched) > > <https://reviews.apache.org/r/64955/diff/2/?file=1931086#file1931086line165> > > > > Why do we add hash, if it already contains it? This comment is relevent for the new patch submitted as this code changed. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64955/#review194953 ----------------------------------------------------------- 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. 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. There are three test failures which need code > change done for SENTRY-2113. > > > Thanks, > > kalyan kumar kalvagadda > >
