----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59508/#review180042 -----------------------------------------------------------
I still see a lot of issues with code style even after you applied formatting to some files. For tests, especially new ones, please document what test cases are actually testing. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java Lines 253 (patched) <https://reviews.apache.org/r/59508/#comment255040> What is the reason for changing SentryInvalidInputException to SentryPluginException? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 57 (patched) <https://reviews.apache.org/r/59508/#comment255042> hiveServerName is completely confusing name for this variable. Also, please add comment explaining what that this server name actually means. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 96 (original), 62 (patched) <https://reviews.apache.org/r/59508/#comment255041> It would be nice to refctor test constructor to call real constructor instead of duplicating code. Also, please document both constructors, especially test-only constructor. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 124 (original), 80 (patched) <https://reviews.apache.org/r/59508/#comment255051> Do we want to set client to null as well? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 364 (original), 167 (patched) <https://reviews.apache.org/r/59508/#comment255050> Interesting - this is done as two separate transactions, so it is possible to write snapshot but fail writing latest snapshot ID. this seems wrong. Do we have existing JIRA filed for this problem? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 627 (original), 213 (patched) <https://reviews.apache.org/r/59508/#comment255056> Hmm - this look svery strange. Why do we only call `persistLastProcessedNotificationID` when notification is *not* process? Shouldn't we call it as well when notification *is* processed? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 232 (patched) <https://reviews.apache.org/r/59508/#comment255054> What is the point in having this "wrapper"? Seems rather useless. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 253 (patched) <https://reviews.apache.org/r/59508/#comment255055> What is the point in having this "wrapper"? Seems rather useless. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Line 34 (original), 58 (patched) <https://reviews.apache.org/r/59508/#comment255059> applies *to* sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Line 46 (original), 75 (patched) <https://reviews.apache.org/r/59508/#comment255060> Please add javadoc. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 101 (patched) <https://reviews.apache.org/r/59508/#comment255063> This doc isn't very useful and doesn't really reflect/explain what the code is doing. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 113 (patched) <https://reviews.apache.org/r/59508/#comment255064> according to javadoc this method is the same as the previous but it isn't. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java Lines 54 (patched) <https://reviews.apache.org/r/59508/#comment255066> Should it implement AutoCloseable? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java Lines 71 (patched) <https://reviews.apache.org/r/59508/#comment255068> Do we actually need this? We can have a test-only constructor that accepts the client as parameter sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java Lines 132 (patched) <https://reviews.apache.org/r/59508/#comment255071> We don't need to do this on every connect - this can be done just once. But we can fix this separately from this refactoring. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java Line 47 (original), 57 (patched) <https://reviews.apache.org/r/59508/#comment255073> Please document what this and other tests are actually testing. - Alexander Kolbasov On July 10, 2017, 3:14 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59508/ > ----------------------------------------------------------- > > (Updated July 10, 2017, 3:14 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/PathImageRetriever.java > de94743 > > 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/PathsImage.java > fd56ce2 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 1402ab1 > > 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/NotificationProcessor.java > 6762de7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java > 9c3e485 > > 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 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestSentryHMSClient.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/59508/diff/8/ > > > Testing > ------- > > Made sure that all the tests around HMSFollower and SentryStore passed. Added > new classes to test new classes added. > > > Thanks, > > kalyan kumar kalvagadda > >
