Re: Review Request 34734: Patch for KAFKA-2226

[Onur Karaman]

---
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 NPE 
(earlier attempts before this patch took anywhere from 5 - 30 attempts to hit 
the NPE):
```
./bin/kafka-run-class.sh kafka.TestPurgatoryPerformance --key-space-size 10 
--keys 3 --num 10 --pct50 50 --pct75 75 --rate 1000 --size 50 --timeout 20
```

As a side note, I think the timing wheel design would be simpler if:
1. we allow TimerTasks to only be run at most once
2. we force TimerTask to have exactly one TimerTaskEntry and TimerTaskEntry to 
only ever belong to exactly one TimerTask (just make the TimerTaskEntry in the 
TimerTask's constructor).

- Onur Karaman


On May 29, 2015, 10:10 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, 10:10 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
> 
>

Re: Review Request 34734: Patch for KAFKA-2226

[Guozhang Wang]

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


On May 29, 2015, 10:10 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, 10:10 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
> 
>

Re: Review Request 34734: Patch for KAFKA-2226

[Jun Rao]

---
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 performance degradation when 
running TestPurgatoryPerformance?

- Jun Rao


On May 29, 2015, 10:10 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, 10:10 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
> 
>

Re: Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

---
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
https://issues.apache.org/jira/browse/KAFKA-2226


Repository: kafka


Description
---

fix a race condition in TimerTaskEntry.remove


Diffs (updated)
-

  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

Re: Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

---
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
https://issues.apache.org/jira/browse/KAFKA-2226


Repository: kafka


Description
---

fix a race condition in TimerTaskEntry.remove


Diffs (updated)
-

  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

Re: Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

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


- Yasuhiro


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34734/#review85764
---


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

Re: Review Request 34734: Patch for KAFKA-2226

[Jun Rao]

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


canceled => cancelled



core/src/main/scala/kafka/utils/timer/Timer.scala


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


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


canceled -> cancelled



core/src/main/scala/kafka/utils/timer/TimingWheel.scala


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

Re: Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

> 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 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.
> 
> Yasuhiro Matsuda wrote:
> I will remove this.

On second thought, I will leave this because this doesn't harm, and this 
ensures the consistency of the data structure without depending on callers to 
do the right thing.


- Yasuhiro


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34734/#review85596
---


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

Re: Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

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

I will remove this.


- Yasuhiro


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34734/#review85596
---


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

Re: Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

---
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 (updated)
-

  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

Re: Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

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

Oh. You are right. I am not sure what I was thinking.


> On May 29, 2015, 1:56 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala, line 153
> > 
> >
> > Not sure if I follow this comment.

I meant "To cancel a task, it should be removed by calling cancel() to prevent 
it from reinsert."


> On May 29, 2015, 1:56 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/utils/timer/TimerTaskList.scala, lines 136-137
> > 
> >
> > With this canceled flag, the logic is a bit more complicated since a 
> > few other places need to check this flag. Not sure how much this helps in 
> > reducing the probability of having a cancelled operation reinserted into 
> > the list. Do you think it's worth doing this?

I believe that this will significantly eliminate canceled task being reinserted 
to a timing wheel or submitted to the task executor.


- Yasuhiro


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34734/#review85665
---


On May 29, 2015, 12:19 a.m., Yasuhiro Matsuda wrote:
> 
> ---
> 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
> 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
> 
>

Re: Review Request 34734: Patch for KAFKA-2226

[Jun Rao]

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


core/src/main/scala/kafka/utils/timer/TimerTaskList.scala


Could you explain a bit how the extra sync helps?



core/src/main/scala/kafka/utils/timer/TimerTaskList.scala


Since this is under synchronized, it seems that remove should always return 
true?



core/src/main/scala/kafka/utils/timer/TimerTaskList.scala


With this canceled flag, the logic is a bit more complicated since a few 
other places need to check this flag. Not sure how much this helps in reducing 
the probability of having a cancelled operation reinserted into the list. Do 
you think it's worth doing this?



core/src/main/scala/kafka/utils/timer/TimerTaskList.scala


Not sure if I follow this comment.


- Jun Rao


On May 29, 2015, 12:19 a.m., Yasuhiro Matsuda wrote:
> 
> ---
> 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
> 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
> 
>

Re: Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

---
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
https://issues.apache.org/jira/browse/KAFKA-2226


Repository: kafka


Description
---

fix a race condition in TimerTaskEntry.remove


Diffs (updated)
-

  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

Re: Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

> 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 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.
> 
> Yasuhiro Matsuda wrote:
> You are right. It should be rare, but a completed entry can remain in the 
> list until expiration. The completion flag in DelayedOperation prevents 
> excess executions. So, it is not too bad.
> 
> Jun Rao wrote:
> Thanks for the clarification. Thinking about this a bit more, could we 
> hit a NullPointerException in step (3)? At that point, list could be null 
> when we call remove.
> 
> Onur Karaman wrote:
> Yeah that check-then-act should probably be done atomically. Maybe all 
> changes/check-then-acts to TimerTaskEntry just need to be guarded by the 
> TimerTaskEntry's intrinsic lock?
> 
> Yasuhiro Matsuda wrote:
> Oops. The value of TimerTaskEntry.list should be save to a local variable.
> 
> Jun Rao wrote:
> Ok, thanks. Perhaps we can also add some comments explaining why we need 
> the while loop and the rare possibility of not removing a completed operation 
> from the Timer.
> 
> Guozhang Wang wrote:
> Could it happen that concurrent threads are calling 
> TimerTaskList.add(entry) and TimerTaskList.remove(entry) on different lists 
> for the same entry? Since we do not synchronize on the entry object this 
> could cause race condition on next/prev/list reference manipulation.

I will add a flag to TimerTaskEntry for cancelation to so that a canceled task 
won't be reinserted. This should reduce a chance of leaving completed task in a 
list.

Also I will add sync on TimerTaskEntry to TimerTaskList.{add|remove}. This 
makes one operation sync on both TimerTaskList and TimerTaskEntry. We have to 
be careful in ordering them, otherwise it may cause a deadlock. I hope I did it 
right. Please review my next patch carefully.


- Yasuhiro


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34734/#review85596
---


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

Re: Review Request 34734: Patch for KAFKA-2226

[Guozhang Wang]

> 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 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.
> 
> Yasuhiro Matsuda wrote:
> You are right. It should be rare, but a completed entry can remain in the 
> list until expiration. The completion flag in DelayedOperation prevents 
> excess executions. So, it is not too bad.
> 
> Jun Rao wrote:
> Thanks for the clarification. Thinking about this a bit more, could we 
> hit a NullPointerException in step (3)? At that point, list could be null 
> when we call remove.
> 
> Onur Karaman wrote:
> Yeah that check-then-act should probably be done atomically. Maybe all 
> changes/check-then-acts to TimerTaskEntry just need to be guarded by the 
> TimerTaskEntry's intrinsic lock?
> 
> Yasuhiro Matsuda wrote:
> Oops. The value of TimerTaskEntry.list should be save to a local variable.
> 
> Jun Rao wrote:
> Ok, thanks. Perhaps we can also add some comments explaining why we need 
> the while loop and the rare possibility of not removing a completed operation 
> from the Timer.

Could it happen that concurrent threads are calling TimerTaskList.add(entry) 
and TimerTaskList.remove(entry) on different lists for the same entry? Since we 
do not synchronize on the entry object this could cause race condition on 
next/prev/list reference manipulation.


- Guozhang


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34734/#review85596
---


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

Re: Review Request 34734: Patch for KAFKA-2226

[Jun Rao]

> 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 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.
> 
> Yasuhiro Matsuda wrote:
> You are right. It should be rare, but a completed entry can remain in the 
> list until expiration. The completion flag in DelayedOperation prevents 
> excess executions. So, it is not too bad.
> 
> Jun Rao wrote:
> Thanks for the clarification. Thinking about this a bit more, could we 
> hit a NullPointerException in step (3)? At that point, list could be null 
> when we call remove.
> 
> Onur Karaman wrote:
> Yeah that check-then-act should probably be done atomically. Maybe all 
> changes/check-then-acts to TimerTaskEntry just need to be guarded by the 
> TimerTaskEntry's intrinsic lock?
> 
> Yasuhiro Matsuda wrote:
> Oops. The value of TimerTaskEntry.list should be save to a local variable.

Ok, thanks. Perhaps we can also add some comments explaining why we need the 
while loop and the rare possibility of not removing a completed operation from 
the Timer.


- Jun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34734/#review85596
---


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

Re: Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

> 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 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.
> 
> Yasuhiro Matsuda wrote:
> You are right. It should be rare, but a completed entry can remain in the 
> list until expiration. The completion flag in DelayedOperation prevents 
> excess executions. So, it is not too bad.
> 
> Jun Rao wrote:
> Thanks for the clarification. Thinking about this a bit more, could we 
> hit a NullPointerException in step (3)? At that point, list could be null 
> when we call remove.
> 
> Onur Karaman wrote:
> Yeah that check-then-act should probably be done atomically. Maybe all 
> changes/check-then-acts to TimerTaskEntry just need to be guarded by the 
> TimerTaskEntry's intrinsic lock?

Oops. The value of TimerTaskEntry.list should be save to a local variable.


- Yasuhiro


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34734/#review85596
---


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

Re: Review Request 34734: Patch for KAFKA-2226

[Onur Karaman]

> 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 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.
> 
> Yasuhiro Matsuda wrote:
> You are right. It should be rare, but a completed entry can remain in the 
> list until expiration. The completion flag in DelayedOperation prevents 
> excess executions. So, it is not too bad.
> 
> Jun Rao wrote:
> Thanks for the clarification. Thinking about this a bit more, could we 
> hit a NullPointerException in step (3)? At that point, list could be null 
> when we call remove.

Yeah that check-then-act should probably be done atomically. Maybe all 
changes/check-then-acts to TimerTaskEntry just need to be guarded by the 
TimerTaskEntry's intrinsic lock?


- Onur


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34734/#review85596
---


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

Re: Review Request 34734: Patch for KAFKA-2226

[Jun Rao]

> 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 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.
> 
> Yasuhiro Matsuda wrote:
> You are right. It should be rare, but a completed entry can remain in the 
> list until expiration. The completion flag in DelayedOperation prevents 
> excess executions. So, it is not too bad.

Thanks for the clarification. Thinking about this a bit more, could we hit a 
NullPointerException in step (3)? At that point, list could be null when we 
call remove.


- Jun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34734/#review85596
---


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

Re: Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

> 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 construction time. So, can the 
> > TaskEntry in the TimerTask ever change?

It can happen if a TimerTask already in a timer is submitted again or submitted 
to another timer. We never do such a thing. But the code handle such uses just 
in case.


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

You are right. It should be rare, but a completed entry can remain in the list 
until expiration. The completion flag in DelayedOperation prevents excess 
executions. So, it is not too bad.


- Yasuhiro


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34734/#review85596
---


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

Re: Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

> 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 TimerTaskEntry should not exist in any list. If it does, removing 
it from the existing list keeps the structure consistent. That is why I added 
the remove call in the add method.
For removing, there is a race condition. When a bucket expires, an entry in the 
bucket is either expired or moved to a finer grain wheel. TimerTaskEntry.remove 
is called then. Then the race condition happens if TimerTask.cancel is called 
at the same time. The remove operation is synchronized on a TimerTaskList 
instance. Therefore, in the syncrinized block, we have to doublecheck that the 
entry still belongs to the list. If the mehod doesn't remove the entry from the 
list due to the race condition, it will retry.


- Yasuhiro


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34734/#review85530
---


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

Re: Review Request 34734: Patch for KAFKA-2226

[Jun Rao]

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


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


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

Re: Review Request 34734: Patch for KAFKA-2226

[Onur Karaman]

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

- Onur Karaman


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

Review Request 34734: Patch for KAFKA-2226

[Yasuhiro Matsuda]

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