> On April 17, 2017, 10:42 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 258 (patched) > > <https://reviews.apache.org/r/58481/diff/2/?file=1693243#file1693243line258> > > > > size() may be expensive operation for some collection - it is better to > > use isEmpty(). > > > > Also, if there are no events we are not going to process any new ones, > > so does it make sense to log? > > > > I would suggest > > > > if ((currentEventID != lastLoggedEventId) && > > response.getEvents().isEmpty()) {
in my approach, for example, the log messages will be CurrentEventId = 2, Processing 2 events CurrentEventId = 4, processing 0 events CurrentEventId = 4, processing 3 events in your approach, the log messages will be CurrentEventId = 2, Processing 2 events CurrentEventId = 4, processing 3 events With this output, we don't need to track lastLoggedEventId. When the set is not empty, the ID will increase in normal case. If it stays the same, something is wrong, and we should log it. - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58481/#review172144 ----------------------------------------------------------- On April 17, 2017, 5:56 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58481/ > ----------------------------------------------------------- > > (Updated April 17, 2017, 5:56 p.m.) > > > Review request for sentry. > > > Bugs: SENTRY-1674 > https://issues.apache.org/jira/browse/SENTRY-1674 > > > Repository: sentry > > > Description > ------- > > Do not log message when CurrentEventID does not change and there is no > updates for HMSFollower > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 16676fb > > > Diff: https://reviews.apache.org/r/58481/diff/2/ > > > Testing > ------- > > > Thanks, > > Na Li > >
