> 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.
No it is not. lina is looking into it. > 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() 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. > 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? On becoming leader, SentryHmsClient would open new connection using hiveConnectionFactory. > 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 141 (patched) > > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line141> > > > > 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? will remove > 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 178 (patched) > > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line178> > > > > Why are we printing it to stdout rather then to the log? will Change. > 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 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. > 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 213 (patched) > > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line213> > > > > and wake up any waitng clients WIll fix > 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 220 (patched) > > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line220> > > > > initialize it to false I removed the initialization to handle the code style errors. Will fix > 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 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. > 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 255 (patched) > > <https://reviews.apache.org/r/59508/diff/9/?file=1774882#file1774882line255> > > > > 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 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. - kalyan kumar ----------------------------------------------------------- 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 > >
