> 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). > > Alexander Kolbasov wrote: > 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.
What will happen if for some reason HMSFOllower isn't processing updates? In responses would be non-empty and it will always print log the message on every invocation which isn't what we want - 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 > >
