----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34734/#review85596 -----------------------------------------------------------
Thanks for the patch. A couple of questions below. Also, in TimerTask.setTimerTaskEntry(), the comment suggests that the TaskEntry can change for a TimerTask. However, it seems that we set the entry for the task only during entry construction time. So, can the TaskEntry in the TimerTask ever change? core/src/main/scala/kafka/utils/timer/TimerTaskList.scala <https://reviews.apache.org/r/34734/#comment137192> Could you explain a bit why this is needed? It seems that we can add the entry either when it's created for the first time or when it's removed from the current list and needs to be added to a new list during reinsert. In both cases, the list in the entry will be null and there is no need to remove the entry from the list. core/src/main/scala/kafka/utils/timer/TimerTaskList.scala <https://reviews.apache.org/r/34734/#comment137190> So, I guess the race condition is the following. The expiration thread moves a TimerTaskEntry from one TimerTaskList to another during reinsert. At the same time, another thread can complete an operation and try to remove the entry from the list. Is that right? With the patch, it seems when TimerTask.cancel tries to re move an entry from the list, the following can happen (1) line 133 tests that list in entry is not null, (2) a reinsert process happens and the entry is removed from list which sets list in the entry to null, (3) list.remove in 134 is called and the entry is not removed since list is now null, (4) line 133 is tested again and since list is now null, we quit the loop, (5) the reinsert process adds the entry to a new list. At this point, a completed entry still exists in the list. - Jun Rao On May 27, 2015, 9 p.m., Yasuhiro Matsuda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34734/ > ----------------------------------------------------------- > > (Updated May 27, 2015, 9 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2226 > https://issues.apache.org/jira/browse/KAFKA-2226 > > > Repository: kafka > > > Description > ------- > > fix a race condition in TimerTaskEntry.remove > > > Diffs > ----- > > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala > e7a96570ddc2367583d6d5590628db7e7f6ba39b > > Diff: https://reviews.apache.org/r/34734/diff/ > > > Testing > ------- > > > Thanks, > > Yasuhiro Matsuda > >
