> On June 11, 2015, 1:07 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/DelayedOperation.scala, lines 264-266 > > <https://reviews.apache.org/r/35201/diff/2/?file=980805#file980805line264> > > > > Not sure if we need this check. Since all writes to watchersForKey are > > sync-ed, it's ok to remove a watcher as long as its count is 0. > > > > I am bit concerned about the overhead on the removeWatchersLock, which > > is global. For example, if you have 1000 requests/sec and each request has > > 1000 partitions, that lock is going to be access 1million times in a sec. > > Could you do some tests/profiling before and after we introduced the global > > lock to see if this could be an issue? > > Jiangjie Qin wrote: > The lock is only grabbed when a watcher has no operations in it. But I > agree that could be an issue. > I'm wondering is there a reason that we have to remove a watcher > immediately when its count become zero? Can we just let the reaper remove > empty watchers? > > Onur Karaman wrote: > +1 on letting the reaper do it.
Both tryCompleteElseWatch() and checkAndComplete() need to grab the global read lock. This is my main concern since this means every produce/fetch request pays a synchronization overhead on a single semaphore for every partition in the request. Some performance testing on the impact of this will be useful. - Jun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35201/#review87495 ----------------------------------------------------------- On June 8, 2015, 6:47 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35201/ > ----------------------------------------------------------- > > (Updated June 8, 2015, 6:47 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2253 > https://issues.apache.org/jira/browse/KAFKA-2253 > > > Repository: kafka > > > Description > ------- > > Incorporated Jiangjie and Onur's comments > > > Diffs > ----- > > core/src/main/scala/kafka/server/DelayedOperation.scala > 123078d97a7bfe2121655c00f3b2c6af21c53015 > > Diff: https://reviews.apache.org/r/35201/diff/ > > > Testing > ------- > > > Thanks, > > Guozhang Wang > >
