> On June 4, 2017, 10:03 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 128 (patched) > > <https://reviews.apache.org/r/59508/diff/3/?file=1738638#file1738638line128> > > > > This looks suspicious - why we are not using exceptions here to report > > errors?
I think Kalyan's reason is: 1) all notification from meta store are valid. If Sentry cannot handle it, throwing exception will cause any difference to meta store. We will swallow it eventually. 2) Throwing exception may stop processing next notification event. The alternative to Kalyan's approach is to throw exception at each event processing. And catch it in the loop of processing the events, log such exception, and move on to next event processing. So we have more info on what's going on. - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59508/#review176885 ----------------------------------------------------------- On June 1, 2017, 6:04 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59508/ > ----------------------------------------------------------- > > (Updated June 1, 2017, 6:04 p.m.) > > > Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, > Sergio Pena, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1769 > https://issues.apache.org/jira/browse/SENTRY-1769 > > > Repository: sentry > > > Description > ------- > > Things included in refactoring. > 1. Moved the complete notification processing logic to notificationProcessor. > 2. Added new class HMSFollowerHelper class which does > HMS Client creation > HMS Client closure > Creating and persisting the HMS snapshot > 3. Misc cleanup > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > 0bd0833 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java > 0d54548 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > cb05a84 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > d410a6c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java > de8e2f7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java > 215f7d5 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > 74a5afb > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java > 1ed92ea > > > Diff: https://reviews.apache.org/r/59508/diff/3/ > > > Testing > ------- > > Made sure that all the tests around HMSFollower and SentryStore passed. > > > Thanks, > > kalyan kumar kalvagadda > >
