----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59508/#review176885 -----------------------------------------------------------
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/#comment250413> This looks suspicious - why we are not using exceptions here to report errors? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 49 (patched) <https://reviews.apache.org/r/59508/#comment250391> This class is not documented - please explain what is this class doing. Also formatting - the class doesn't start on the first column sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 55 (patched) <https://reviews.apache.org/r/59508/#comment250392> It is usually better to separte object construction from connection establishement. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 78 (patched) <https://reviews.apache.org/r/59508/#comment250399> Can we handle simple (non-kerberos) case first and then deal with more complicated kerberos case without if clause? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 144 (patched) <https://reviews.apache.org/r/59508/#comment250400> Note that this may throw exception preventing kerberos context from being closed. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 167 (patched) <https://reviews.apache.org/r/59508/#comment250401> The name suggests that there is a problem here. Why combine two separate operations into one? I think there should be fetchSnapshot() and storeSnapshot() - the first one belongs here, the second one doesn't. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 208 (patched) <https://reviews.apache.org/r/59508/#comment250402> We only need to store full snapshot if the event ID didn't change in the process sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 235 (patched) <https://reviews.apache.org/r/59508/#comment250404> What are "new notifications" This probably should be "Get all HMS notifications since specified one" sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 241 (patched) <https://reviews.apache.org/r/59508/#comment250403> All methods here should probably be package-private rather then public sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 247 (patched) <https://reviews.apache.org/r/59508/#comment250405> reverse the condition and return immediately if it isn't. if (eventId.getEventId() <= lastProcessedNotificationID) { return Collections.emptyList(); } sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 251 (patched) <https://reviews.apache.org/r/59508/#comment250406> Combine the two conditions into one if statement if (response.isSetEvents() && (!response.getEvents().isEmpty() && (lastProcessedNotificationID != lastLoggedEventId))) { sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 72 (original), 41 (patched) <https://reviews.apache.org/r/59508/#comment250395> Do we still need this? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 75 (original), 44 (patched) <https://reviews.apache.org/r/59508/#comment250394> This comment is now detached from the variable sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 111 (original), 77 (patched) <https://reviews.apache.org/r/59508/#comment250396> Should check for (client != null) sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 256 (original), 123 (patched) <https://reviews.apache.org/r/59508/#comment250397> Combine declaration with assignment sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 257 (original), 124 (patched) <https://reviews.apache.org/r/59508/#comment250398> What does it mean - create a full snapshot if there is one? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 78 (patched) <https://reviews.apache.org/r/59508/#comment250414> Why not just this.conf? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 111 (patched) <https://reviews.apache.org/r/59508/#comment250415> Combine declaraton with assignment sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 126 (patched) <https://reviews.apache.org/r/59508/#comment250416> These guys can be cached at constructor sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java Lines 225 (patched) <https://reviews.apache.org/r/59508/#comment250410> What does it mean "constructs name"? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java Lines 227 (patched) <https://reviews.apache.org/r/59508/#comment250411> It looks like the callers do not bother tocheck if it is null. Should it return an empty string itself? Or throw exception? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java Lines 230 (patched) <https://reviews.apache.org/r/59508/#comment250407> try pronouncing authzble over the phone :) sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java Lines 231 (patched) <https://reviews.apache.org/r/59508/#comment250412> How about this: public static String getAuthzObj(TSentryAuthorizable authorizable) { String dbName = authorizable.getDb(); if (SentryStore.isNULL(dbName)) { return null; } String tblName = authorizable.getTable(); return SentryStore.isNULL(tblName) ? dbName.toLowerCase() : (dbName + "." + authorizable.getTable()).toLowerCase(); } sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java Lines 232 (patched) <https://reviews.apache.org/r/59508/#comment250408> return null immediately if getDb is null sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java Line 222 (original), 243 (patched) <https://reviews.apache.org/r/59508/#comment250409> Missing newline - Alexander Kolbasov 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 > >
