----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34734/#review85764 -----------------------------------------------------------
Thanks for the new patch. A few more comments below. core/src/main/scala/kafka/utils/timer/Timer.scala <https://reviews.apache.org/r/34734/#comment137523> canceled => cancelled core/src/main/scala/kafka/utils/timer/Timer.scala <https://reviews.apache.org/r/34734/#comment137522> Our current style is to not wrap single-line statement in brackets. Ditto for a few places below. core/src/main/scala/kafka/utils/timer/TimerTaskList.scala <https://reviews.apache.org/r/34734/#comment137548> It seems that if the task entry is still in another list during the add call, a deadlock can happen. Suppose that a task entry is initially in taskList1. The expiration thread tries to remove the task entry from taskList1 and to insert it into taskList2. The call gets all the way to before line 68. The expiration thread is holding a lock on the task entry and taskList2. Now, another thread thread1 tries to remove the task entry from taskList1. It grabs the lock on taskList1 and then tries to acquire the lock on the task entry, but can't since the expiration thread is holding it. The expiration thread resumes in line 68 and tries to grab the lock on taskList1, but can't since thread1 is holding it. Now, we are in a deadlock. It seems that this won't happen in our usage since we always remove an existing task entry from a list before reinserting it to another list. Because of this, add() will never need to hold the lock on two task lists. Not sure if it's better to just enforce this assumption or make the code more general than we currently need. If we do the former, not sure if we still need to double sync on the list and the task entry. core/src/main/scala/kafka/utils/timer/TimerTaskList.scala <https://reviews.apache.org/r/34734/#comment137519> canceled -> cancelled core/src/main/scala/kafka/utils/timer/TimingWheel.scala <https://reviews.apache.org/r/34734/#comment137520> canceled -> cancelled - Jun Rao On May 29, 2015, 5:49 p.m., Yasuhiro Matsuda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34734/ > ----------------------------------------------------------- > > (Updated May 29, 2015, 5:49 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/Timer.scala > b8cde820a770a4e894804f1c268b24b529940650 > core/src/main/scala/kafka/utils/timer/TimerTask.scala > 3407138115d579339ffb6b00e32e38c984ac5d6e > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala > e7a96570ddc2367583d6d5590628db7e7f6ba39b > core/src/main/scala/kafka/utils/timer/TimingWheel.scala > e92aba3844dbf3372182e14536a5d98cf3366d73 > > Diff: https://reviews.apache.org/r/34734/diff/ > > > Testing > ------- > > > Thanks, > > Yasuhiro Matsuda > >
