----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59508/#review178434 -----------------------------------------------------------
Thanks Kalyan, this is an interesting refactoring, thanks for the hard work your putting here. Btw, I have the feeling that this refactor can be splitted into 3 different patches; the HMSClientWrapper, the NotificationProcessor and the HMSFollower itself. Shouldn't it be better to split this to allow better code reviews? It's just a recommendation and is optional. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java Lines 45 (patched) <https://reviews.apache.org/r/59508/#comment252393> Unused import. 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/#comment252394> Empty space/tab. This is causing the file to be shown in the patch even if it does nothing to do with the change. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 71 (patched) <https://reviews.apache.org/r/59508/#comment252396> I think this should be setHMSClient() as it is just doing that. Makes more sense I think. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 82 (patched) <https://reviews.apache.org/r/59508/#comment252397> Put a space between the operator 'return client != null' or you may use 'return (client != null)'. Typical coding conventions used in Apache projects is to have this space in the operators. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 206 (patched) <https://reviews.apache.org/r/59508/#comment252398> If the fetchFullUpdate() snapshot ignores the failure, why we have to throw an exception here? This might confuse users when there are empty snapshots, right? Isn't better just to ignore the error and return an empty map list as well? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 251 (patched) <https://reviews.apache.org/r/59508/#comment252399> A typo on Get's. Should be Gets. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 260 (patched) <https://reviews.apache.org/r/59508/#comment252400> getFullSnapShot() throws an exception if the HMS is not connected, and this function just returns an empty list. We should be consistent about what to do when calling methods that require an HMS client connected. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 221 (original), 109 (patched) <https://reviews.apache.org/r/59508/#comment252401> Is this method needed now? Seems it is not used anywhere. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 533 (original), 224 (patched) <https://reviews.apache.org/r/59508/#comment252402> Shouldn't this condition happen early? Perhaps after checking if events.isEmpty()? Seems odd that we might read the hive-site.xml, instatiate the notificationProcessor, and then exit on the first event because the leaderMonitor is not the leader. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 648 (original), 270 (patched) <https://reviews.apache.org/r/59508/#comment252403> Couldn't be this a IOException? It is attempting to persist (write/output) to a DB, so it looks like IOException make sense. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 288 (patched) <https://reviews.apache.org/r/59508/#comment252404> Couldn't be this a IOException? It is attempting to persist (write/output) to a DB, so it looks like IOException make sense. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java Lines 20 (patched) <https://reviews.apache.org/r/59508/#comment252405> Seems the naming convention for Sentry exceptions are that classes start with Sentry###Exception. Should we follow the same idea? Also, why not having a generic exception instead of two (HMSFollowerPersistenceException and HMSFollowerSnapShotCreatingException)? Both refer to problems with the HMSFollower, and they only wrap a message. Btw, is there a common Sentry###Exception we could use instead of creating an new one? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 75 (patched) <https://reviews.apache.org/r/59508/#comment252406> Need a space. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 85 (patched) <https://reviews.apache.org/r/59508/#comment252407> Need to fix indentation. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 96 (patched) <https://reviews.apache.org/r/59508/#comment252408> Remove space before the parethesis. "setAuthzConf (Configuration conf)" Btw, if this method exists now, should we call it from the constructor? Seems we are duplicating code here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 148 (patched) <https://reviews.apache.org/r/59508/#comment252409> Shouldn't this be a warning instead of error? Error implies that something bad happened on the code. Here, we're just ignoring these invalid requests. A warning is more appropiate. The same for the rest of the notifications requests. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java Lines 37 (patched) <https://reviews.apache.org/r/59508/#comment252410> Need a space here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java Lines 233 (patched) <https://reviews.apache.org/r/59508/#comment252395> What's wrong with the the old getAuthzObj implementation? The old method was returning NULL instead of throwing an exception, something that is less expensive if it occurs very often. Also, it makes sense to return a NULL if the authzble object has as NULL db, right? The caller should handle the NULL value, and decide what to do with it instead of catching an exception. - Sergio Pena On June 20, 2017, 4:11 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59508/ > ----------------------------------------------------------- > > (Updated June 20, 2017, 4:11 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 > 8b19c88 > > 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 > 1f7eb18 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerSnapShotCreationException.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java > 6762de7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java > PRE-CREATION > > 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/TestHMSClientWrapper.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > 66ad2a1 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/59508/diff/4/ > > > Testing > ------- > > Made sure that all the tests around HMSFollower and SentryStore passed. Added > new classes to test new classes added. > > > Thanks, > > kalyan kumar kalvagadda > >
