----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59508/#review176831 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java Line 24 (original) <https://reviews.apache.org/r/59508/#comment250306> Extra space. Please remove it from the patch. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 49 (patched) <https://reviews.apache.org/r/59508/#comment250309> The name does not sound correct now that I think of. HMSClientWrapper is not only wrapping HMS calls, it is also precessing notifications and persisting data on Sentry. Shouldn't be better to have a HiveSnapshotHelper instead? class HiveSnapshotHelper { /* save conf and sentrystore for reference */ public HiveSnapshotHelper(Configuration conf, SentryStore sentryStore); /* fetch and persist a new snapshot; return true if persisted, false if not (throw exceptions on errors) */ public boolean createAndPersistSnapshot(); /* fetch and process new notifications; return true if persisted, false if not (throw exceptions on errors) */ public boolean processNewNotifications(); public void close(); } Also, how can we re-connect to HMS if close() is called? I see that HMSFollower just creates another instance, but being a public class, shouldn't we have an better interface or contract to do that? probably having a connect()/isConnected() methods? or make the important methods to re-connect if it is not connected? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 54 (patched) <https://reviews.apache.org/r/59508/#comment250308> Add a space here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 190 (patched) <https://reviews.apache.org/r/59508/#comment250310> This isn't an error. An empty snapshot is a normal thing, why should we treat this as a failure? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 211 (patched) <https://reviews.apache.org/r/59508/#comment250311> Shouldn't we persist the latest notification ID in this method? Persisting a snapshot + notification ID are related, aren't they? If so, then the method should do it here instead of expecting the caller does it. This would be useful for testing too, the tests should only call createAndPersistSnapshot() and just check everything is correct. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 246 (original), 115 (patched) <https://reviews.apache.org/r/59508/#comment250307> Do we need this variable? Isn't enough to check if client != null to verify if the HMS client is connected? - Sergio Pena 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 > >
