----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59508/#review178922 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java Lines 255 (patched) <https://reviews.apache.org/r/59508/#comment253322> just use failure, drop failure,.toString() sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 69 (patched) <https://reviews.apache.org/r/59508/#comment253286> This doesn't initialize anything - it is a simple setter, so probably should be just setClient() sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 104 (patched) <https://reviews.apache.org/r/59508/#comment253287> We can return immediately here sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 188 (patched) <https://reviews.apache.org/r/59508/#comment253249> Signature shows that it throws Exception, it is inconsistent with the doc sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 190 (patched) <https://reviews.apache.org/r/59508/#comment253248> Snapshot is a single word sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 210 (patched) <https://reviews.apache.org/r/59508/#comment253288> Should we log something in this case as well? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 211 (patched) <https://reviews.apache.org/r/59508/#comment253284> String.format doesn't understand {} format specifiers, only log4j does sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 264 (patched) <https://reviews.apache.org/r/59508/#comment253290> should be sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 267 (patched) <https://reviews.apache.org/r/59508/#comment253292> else is not needed since the previous clause ends with return sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 273 (patched) <https://reviews.apache.org/r/59508/#comment253295> This isn't needed - if it is empty, we return an empty collection anyway. So it can be just return response.isSetEvents() ? response.getEvents() : Collections.<NotificationEvent>emptyList(); sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 275 (patched) <https://reviews.apache.org/r/59508/#comment253294> we are not processing anything here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 84 (original), 48 (patched) <https://reviews.apache.org/r/59508/#comment253311> This isn't hiveServerName, but location(s) of HMS service, so a better name can be hmsLocation. Please add comment explaining why it isn't final since it is very non-trivial. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 221 (original), 110 (patched) <https://reviews.apache.org/r/59508/#comment253312> IntelliJ shows this as not used sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 251 (original), 130 (patched) <https://reviews.apache.org/r/59508/#comment253313> shouldn't we set client to null in this case? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 258 (original), 136 (patched) <https://reviews.apache.org/r/59508/#comment253315> This is rather strange function - we can decide whether it is needed or not and have a function that just returns a full snapshot unconditionally sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 336 (original), 138 (patched) <https://reviews.apache.org/r/59508/#comment253314> Is it always TException? Can it throw some kind of Kerberos exception? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 358 (original), 167 (patched) <https://reviews.apache.org/r/59508/#comment253316> here and below, snapshot is a single word sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 385 (original), 193 (patched) <https://reviews.apache.org/r/59508/#comment253317> Why do we need this function? It isn't called by anything else, so we can just inline it as processNotifications(client.getHMSNotifications( notificationID)) sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 219 (patched) <https://reviews.apache.org/r/59508/#comment253318> Why do we need to create a new NotificationProcessor every time? Why not just create one in constructor? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 622 (original), 235 (patched) <https://reviews.apache.org/r/59508/#comment253319> We are throwing it only to immediately catch in the caller and print an error message - what is the point? 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/#comment253320> Again, we are throwing this exception only to catch it in the same class in another method and pint an error - we can as well do it here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 288 (patched) <https://reviews.apache.org/r/59508/#comment253321> Same comment about throwing exception sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java Lines 20 (patched) <https://reviews.apache.org/r/59508/#comment253247> I still do not see a value in the special exception here - it is only used to show different logs which can be achieved without introducing special exceptions. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 83 (patched) <https://reviews.apache.org/r/59508/#comment253298> why not just syncOnCreate? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 95 (patched) <https://reviews.apache.org/r/59508/#comment253299> I thinuk it is better to have explicit setters for these variables rather then pass this via conf sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 97 (patched) <https://reviews.apache.org/r/59508/#comment253300> Why this is not done the same way as in constructor? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 113 (patched) <https://reviews.apache.org/r/59508/#comment253301> can this be package-private? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 130 (patched) <https://reviews.apache.org/r/59508/#comment253304> Add case INSERT: with whatever shopuld happen there (possibly just return true) sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 132 (patched) <https://reviews.apache.org/r/59508/#comment253305> Log an error because this should never happen once you handle INSERT sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Line 241 (original), 455 (patched) <https://reviews.apache.org/r/59508/#comment253306> Remove these hashes at the beginning sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Line 303 (original), 517 (patched) <https://reviews.apache.org/r/59508/#comment253307> Remove hashes at the beginning sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Line 394 (original), 609 (patched) <https://reviews.apache.org/r/59508/#comment253308> This should be static (as well as getPath() sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 654 (patched) <https://reviews.apache.org/r/59508/#comment253309> What is this function doing? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 663 (patched) <https://reviews.apache.org/r/59508/#comment253310> What is this function doing? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SnapshotCreationException.java Lines 17 (patched) <https://reviews.apache.org/r/59508/#comment253285> I don't think this exception is needed - no one even catches it explicitely! - Alexander Kolbasov On June 22, 2017, 8:28 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59508/ > ----------------------------------------------------------- > > (Updated June 22, 2017, 8:28 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/FullSnapshotInfo.java > PRE-CREATION > > 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/NotificationProcessor.java > 6762de7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java > 215f7d5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SnapshotCreationException.java > PRE-CREATION > > 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/5/ > > > Testing > ------- > > Made sure that all the tests around HMSFollower and SentryStore passed. Added > new classes to test new classes added. > > > Thanks, > > kalyan kumar kalvagadda > >
