----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59508/#review180338 -----------------------------------------------------------
Comments for the HmsFollower class only. Not complete review. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 64 (patched) <https://reviews.apache.org/r/59508/#comment255445> It is ok, but a bit weird - the main constructor is test-only, not another way around. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 82 (patched) <https://reviews.apache.org/r/59508/#comment255448> This should be moved to the `if` case below. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 84 (patched) <https://reviews.apache.org/r/59508/#comment255446> Is it an attempt to fix the problem with missing server name? Are you sure that this will have the setting? If you still use, please introduce a new variable rather then reusing the parameter. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 88 (patched) <https://reviews.apache.org/r/59508/#comment255449> Extra line sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 103 (patched) <https://reviews.apache.org/r/59508/#comment255451> please also include the failure sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 105 (patched) <https://reviews.apache.org/r/59508/#comment255450> It would be good to set client to null as well to allow multiple calls to close() sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 124 (patched) <https://reviews.apache.org/r/59508/#comment255452> This doesn't look right. Once we become a leader, who will re-open the client? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 141 (patched) <https://reviews.apache.org/r/59508/#comment255454> Do we need to do it here? Can we hie this check in the client instead so that we connect as needed before each peration on the client? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 178 (patched) <https://reviews.apache.org/r/59508/#comment255455> Why are we printing it to stdout rather then to the log? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 200 (patched) <https://reviews.apache.org/r/59508/#comment255457> Why are we doing this - why not let the caller to log the failure? This will allow us to remove the whole try-block sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 213 (patched) <https://reviews.apache.org/r/59508/#comment255458> and wake up any waitng clients sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 220 (patched) <https://reviews.apache.org/r/59508/#comment255459> initialize it to false sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 232 (patched) <https://reviews.apache.org/r/59508/#comment255460> this is not needed since it is initialized to false initially sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java Lines 255 (patched) <https://reviews.apache.org/r/59508/#comment255461> Why are we doing this here instead of the caller logging the exception? Since you just re-throw the exception you can as well just remove the try statement and propagate it to the caller - Alexander Kolbasov On July 12, 2017, 5:44 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59508/ > ----------------------------------------------------------- > > (Updated July 12, 2017, 5:44 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 > 2426b40 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > d6100de > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java > 5b8a572 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsSnapshotId.java > d683c2c > > 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 > 4d852e6 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 350c922 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > ad23334 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 2d581f7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.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/SentryHmsClient.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 322197b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java > 9c3e485 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 193c611 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 395cba6 > > 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/TestHmsFollower.java > PRE-CREATION > > 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 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java > b9330cc > > > Diff: https://reviews.apache.org/r/59508/diff/9/ > > > Testing > ------- > > Made sure that all the tests around HMSFollower and SentryStore passed. Added > new classes to test new classes added. > > > Thanks, > > kalyan kumar kalvagadda > >
