----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59508/#review178456 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryHmsNotification.java Line 25 (original), 24 (patched) <https://reviews.apache.org/r/59508/#comment252638> Side note - since the rest is a plain comment (not Javadoc, the use of HTML formatting is completely useless. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 54 (patched) <https://reviews.apache.org/r/59508/#comment252459> should it be final? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 57 (patched) <https://reviews.apache.org/r/59508/#comment252458> Stale comment sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 70 (patched) <https://reviews.apache.org/r/59508/#comment252460> should it be protected or package-private? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 81 (patched) <https://reviews.apache.org/r/59508/#comment252552> This can be package-private sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 92 (patched) <https://reviews.apache.org/r/59508/#comment252590> There are several exceptions listed below - should they be mentioned here as well? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 94 (patched) <https://reviews.apache.org/r/59508/#comment252554> Naming - by definition this is the class talking to HMS, so a simple connect() name should be good enough. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 97 (patched) <https://reviews.apache.org/r/59508/#comment252550> Please declare one variable per line. ALso we only need them if kerberos enabled, so move these closer to the use. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 122 (patched) <https://reviews.apache.org/r/59508/#comment252461> use built-in formatting sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 125 (patched) <https://reviews.apache.org/r/59508/#comment252462> use built-in formatting for preconditions sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 131 (patched) <https://reviews.apache.org/r/59508/#comment252463> use built-in formatting sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 174 (patched) <https://reviews.apache.org/r/59508/#comment252589> This is a problem with the original code - if client.close() throws an exception, the kerberos context will never be shut down. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 190 (patched) <https://reviews.apache.org/r/59508/#comment252626> This doesn't match the signature. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 195 (patched) <https://reviews.apache.org/r/59508/#comment252594> Why do you need this? from the code it seems rather useless. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 198 (patched) <https://reviews.apache.org/r/59508/#comment252608> Do we really need a custom exception here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 201 (patched) <https://reviews.apache.org/r/59508/#comment252595> Brig the declaration to where it is used and combine with assignment sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 205 (patched) <https://reviews.apache.org/r/59508/#comment252604> I don't think it can ever be null sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 211 (patched) <https://reviews.apache.org/r/59508/#comment252605> space before {} sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 224 (patched) <https://reviews.apache.org/r/59508/#comment252464> use built-in formatting sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 251 (patched) <https://reviews.apache.org/r/59508/#comment252627> Even better, it should be "Return all HMS notifications ..." sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 253 (patched) <https://reviews.apache.org/r/59508/#comment252628> Naming - for the purpose of this function we don't care whether it is last or not, we are asked to return all notification from some specified initial value. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 258 (patched) <https://reviews.apache.org/r/59508/#comment252629> This formatting is unusual. Probably should be Collection<NotificationEvent> getHMSNotifications(long lastProcessedNotificationID) throws Exception { sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 260 (patched) <https://reviews.apache.org/r/59508/#comment252630> Also, if client isn't connected, there is no point on constantly throwing the same error message over and over. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 268 (patched) <https://reviews.apache.org/r/59508/#comment252631> This can be combined with previous message into one. Otherwise you need to clarify what is the id received. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 269 (patched) <https://reviews.apache.org/r/59508/#comment252632> The case where they are equal means that there are no new events. But the case when this is smaller means that something strange is happening and warrants a warning. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSClientWrapper.java Lines 277 (patched) <https://reviews.apache.org/r/59508/#comment252465> use built-in formatting sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java Lines 21 (patched) <https://reviews.apache.org/r/59508/#comment252639> Seems like other Sentry exceptions are using unique version IDs. I am not sure how they ar egenerated. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerPersistenceException.java Lines 24 (patched) <https://reviews.apache.org/r/59508/#comment252640> Why do we need this? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollowerSnapShotCreationException.java Lines 19 (patched) <https://reviews.apache.org/r/59508/#comment252606> Why do we need this? Also, if it is needed, we need a better name. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java Lines 26 (patched) <https://reviews.apache.org/r/59508/#comment252617> 1) The class can be final 2) Naming - the name is long and confusing. And Snapshot is not two words but one. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java Lines 29 (patched) <https://reviews.apache.org/r/59508/#comment252619> This isn't last notification ID but notification ID associated with this particular ID, so this is just snapshot ID. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java Lines 37 (patched) <https://reviews.apache.org/r/59508/#comment252623> Naming - getPathsSnapshot() is shorter sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/PathsFullSnapShotInfo.java Lines 41 (patched) <https://reviews.apache.org/r/59508/#comment252624> I'd suggest getId(). Can it be package-private? - Alexander Kolbasov 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 > >
