> On April 18, 2017, 9:15 p.m., Vamsee Yarlagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 275 (patched) > > <https://reviews.apache.org/r/58221/diff/15/?file=1693584#file1693584line307> > > > > I know it doesn't hurt to check for hmsFollower != null but to make the > > code more readable can we replace this constraint with > > (notificationLogEnabled == true)? > > Na Li wrote: > check notification itself is not enough. If hmsFollower is null, we don't > want to continue scheduling its task. > > I was checking both notifcation and hmsFollower. As you mentioned, > checking notificationLogEnabled makes code more readable. But Kalyan wants to > get rid of it to make the code simplier. > > So how can I satisfy both preference? Or you two can reach agreement? > > Na Li wrote: > Sasha mentioned that notificationLogEnabled will be gone soon as it will > always be true. If it is false, Hive and Sentry won't work together.
The reason why I left my comment was - Looking at the code, I realized that hmsFollower can never be null if the constructor of SentryService passed. So i thought it is not really necessarily to check whether it is null or not. But looking at your comment pointing to Sasha's note that this notificationLogEnabled flag will soon go away, then we can leave it as is to avoid making changes again. - Vamsee ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58221/#review172262 ----------------------------------------------------------- On April 19, 2017, 2:23 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58221/ > ----------------------------------------------------------- > > (Updated April 19, 2017, 2:23 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar > kalvagadda, and Sergio Pena. > > > Repository: sentry > > > Description > ------- > > SENTRY-1649 move HMS follower to runServer > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 16676fb > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 132db63 > > > Diff: https://reviews.apache.org/r/58221/diff/17/ > > > Testing > ------- > > > Thanks, > > Na Li > >