> 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()) { > > Na Li wrote: > 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 Li wrote: > I use a variable to remember the value of response.getEvents().size(). > based on > http://stackoverflow.com/questions/24392682/java-list-size-performance-and-tips, > the overhead of List.size() is O(1).
I've read somewhere that for linked lists it isn't true but other docs suggest that it is still O(1), so you are right. IMO isEmpty() more clearly shows the intention, but I drop my perf argument. As for the the two approaches - it seems reasonable both ways so please do whatever you think is best. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58481/#review172144 ----------------------------------------------------------- On April 18, 2017, 4:29 a.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58481/ > ----------------------------------------------------------- > > (Updated April 18, 2017, 4:29 a.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/3/ > > > Testing > ------- > > > Thanks, > > Na Li > >
