> On Sept. 6, 2017, 11:37 p.m., Vamsee Yarlagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > > Lines 98 (patched) > > <https://reviews.apache.org/r/62136/diff/1/?file=1816684#file1816684line100> > > > > Why can't we leave this to HMSFollower to handle this?
I think the HiveNotificationFetcher is the best place to close a connection in case of errors so that other callers do not have to catch exceptions and close connections. My thinking of the HiveNotificationFetcher is that it can be also used by the SentryHMSClient to consolidate notifications when fetching a new snapshot. Also, I am thinkin on refactor the SentryHMSClient to keep te context of snapshots and add the close() internally as well. This way we don't depend on the caller (like HMSFollower) to close connections if something is wrong. - Sergio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62136/#review184757 ----------------------------------------------------------- On Sept. 7, 2017, 1:24 a.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62136/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2017, 1:24 a.m.) > > > Review request for sentry, Alexander Kolbasov and Vamsee Yarlagadda. > > > Bugs: sentry-1928 > https://issues.apache.org/jira/browse/sentry-1928 > > > Repository: sentry > > > Description > ------- > > The patch is closing the HMS connections on the HiveNotificationFetcher if > any Exception occurrs. The HMSFollower has also code to close the connection > on any Exception, but I do it for legacy reasons as I think it is not > required anymore. The HMSFollower needs some refactoring, though. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > d4feb380fa0f3bd5f237609a107295a2d23eae3b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > 4d329921bb4bc540992084687726155cb0373f0f > > > Diff: https://reviews.apache.org/r/62136/diff/2/ > > > Testing > ------- > > > Thanks, > > Sergio Pena > >
