> On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java > > Lines 84 (patched) > > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line84> > > > > 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. > > kalyan kumar kalvagadda wrote: > No it is not. lina is looking into it.
I am assuming you revert to using Sentry config for this. > On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java > > Lines 105 (patched) > > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line105> > > > > It would be good to set client to null as well to allow multiple calls > > to close() > > kalyan kumar kalvagadda wrote: > I don't see a reason to destoy SentryHmsClient. It is constructed when > HMSFollower is constructed and stays. Based on situation close/connect are > called on it. I see how it works now. Dropping the issue. > On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java > > Lines 124 (patched) > > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line124> > > > > This doesn't look right. Once we become a leader, who will re-open the > > client? > > kalyan kumar kalvagadda wrote: > On becoming leader, SentryHmsClient would open new connection using > hiveConnectionFactory. I see. I was confused because usually once you call close() you are not supposed to use an object. May be it is better to have a different method for client for this purpose. > On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java > > Lines 200 (patched) > > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line200> > > > > Why are we doing this - why not let the caller to log the failure? This > > will allow us to remove the whole try-block > > kalyan kumar kalvagadda wrote: > if the caller is not doing anything specific to that excpetion we need > not have handlers for those specific exceptions just to log the failure. > > With the approach i have taken, caller doesn't have any spefic handlers > for these exception and all of then are handled with one block. But the caller logs the exception anyway. Here you are not adding any extra value - just loggin an exception and passing it on. > On July 12, 2017, 7:01 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HmsFollower.java > > Lines 232 (patched) > > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line232> > > > > this is not needed since it is initialized to false initially > > kalyan kumar kalvagadda wrote: > we need it as the isNotificationProcessed can be set to true in the loop. > let's say a case where notification-1 is processed. > isNotificationProcessed will be set to true. > notification-2 is not processed becuase of an exception we need to > explicitly set the isNotificationProcessed to false. Looks like it should be set to false on each iteration? - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59508/#review180338 ----------------------------------------------------------- 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 > >
