Re: Review Request 34734: Patch for KAFKA-2226

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34734/#review86047 --- Ran the patch with the following command 60+ times and didn't see an

Re: Review Request 34734: Patch for KAFKA-2226

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34734/#review85916 --- Ship it! Locally test 10+ runs without seeing NPE. - Guozhang Wang

Re: Review Request 34734: Patch for KAFKA-2226

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34734/#review85905 --- Ship it! Thanks for the latest patch. +1. BTW, is there any perform

Re: Review Request 34734: Patch for KAFKA-2226

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34734/ --- (Updated May 29, 2015, 10:10 p.m.) Review request for kafka. Bugs: KAFKA-2226

Re: Review Request 34734: Patch for KAFKA-2226

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34734/ --- (Updated May 29, 2015, 10:04 p.m.) Review request for kafka. Bugs: KAFKA-2226

Re: Review Request 34734: Patch for KAFKA-2226

> On May 29, 2015, 7:08 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/timer/Timer.scala, line 54 > > > > > > canceled => cancelled I will fix it. By the way, "canceled" is a legitimate spelling in American

Re: Review Request 34734: Patch for KAFKA-2226

--- 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

Re: Review Request 34734: Patch for KAFKA-2226

> On May 28, 2015, 7:10 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala, lines 64-65 > > > > > > Could you explain a bit why this is needed? It seems that we can add > > the entry eit

Re: Review Request 34734: Patch for KAFKA-2226

> On May 28, 2015, 7:10 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala, lines 64-65 > > > > > > Could you explain a bit why this is needed? It seems that we can add > > the entry eit

Re: Review Request 34734: Patch for KAFKA-2226

--- 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

Re: Review Request 34734: Patch for KAFKA-2226

> On May 29, 2015, 1:56 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala, line 106 > > > > > > Since this is under synchronized, it seems that remove should always > > return true? O

Re: Review Request 34734: Patch for KAFKA-2226

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34734/#review85665 --- Thanks for the new patch. A few more clarification questions below.

Re: Review Request 34734: Patch for KAFKA-2226

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34734/ --- (Updated May 29, 2015, 12:19 a.m.) Review request for kafka. Bugs: KAFKA-2226

Re: Review Request 34734: Patch for KAFKA-2226

> On May 28, 2015, 7:10 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala, lines 132-135 > > > > > > So, I guess the race condition is the following. The expiration thread > > moves a

Re: Review Request 34734: Patch for KAFKA-2226

> On May 28, 2015, 7:10 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala, lines 132-135 > > > > > > So, I guess the race condition is the following. The expiration thread > > moves a

Re: Review Request 34734: Patch for KAFKA-2226

> On May 28, 2015, 7:10 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala, lines 132-135 > > > > > > So, I guess the race condition is the following. The expiration thread > > moves a

Re: Review Request 34734: Patch for KAFKA-2226

> On May 28, 2015, 7:10 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala, lines 132-135 > > > > > > So, I guess the race condition is the following. The expiration thread > > moves a

Re: Review Request 34734: Patch for KAFKA-2226

> On May 28, 2015, 7:10 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala, lines 132-135 > > > > > > So, I guess the race condition is the following. The expiration thread > > moves a

Re: Review Request 34734: Patch for KAFKA-2226

> On May 28, 2015, 7:10 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala, lines 132-135 > > > > > > So, I guess the race condition is the following. The expiration thread > > moves a

Re: Review Request 34734: Patch for KAFKA-2226

> On May 28, 2015, 7:10 p.m., Jun Rao wrote: > > 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

Re: Review Request 34734: Patch for KAFKA-2226

> On May 28, 2015, 8:42 a.m., Onur Karaman wrote: > > It seems like you're concerned about adding/removing a TimerTaskEntry that > > already exists in another TimerTaskList. Can you explain how that can > > happen? My understanding of the timing wheel stuff is only so-so. For adding, a TimerTa

Re: Review Request 34734: Patch for KAFKA-2226

--- 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 TimerTa

Re: Review Request 34734: Patch for KAFKA-2226

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34734/#review85530 --- It seems like you're concerned about adding/removing a TimerTaskEntr

Review Request 34734: Patch for KAFKA-2226

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34734/ --- Review request for kafka. Bugs: KAFKA-2226 https://issues.apache.org/jira/b