You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jun Rao <ju...@gmail.com> on 2015/02/14 02:52:12 UTC
Review Request 31040: Patch for kafka-1952
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31040/
-----------------------------------------------------------
Review request for kafka.
Bugs: kafka-1952
https://issues.apache.org/jira/browse/kafka-1952
Repository: kafka
Description
-------
KAFKA-1952; High CPU Usage in 0.8.2 release
Diffs
-----
core/src/main/scala/kafka/server/RequestPurgatory.scala 9d76234bc2c810ec08621dc92bb4061b8e7cd993
Diff: https://reviews.apache.org/r/31040/diff/
Testing
-------
Thanks,
Jun Rao
Re: Review Request 31040: Patch for kafka-1952
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31040/#review72559
-----------------------------------------------------------
Ship it!
Ship It!
- Neha Narkhede
On Feb. 15, 2015, 11:26 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31040/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2015, 11:26 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1952
> https://issues.apache.org/jira/browse/kafka-1952
>
>
> Repository: kafka
>
>
> Description
> -------
>
> address review comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/RequestPurgatory.scala 9d76234bc2c810ec08621dc92bb4061b8e7cd993
>
> Diff: https://reviews.apache.org/r/31040/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 31040: Patch for kafka-1952
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31040/
-----------------------------------------------------------
(Updated Feb. 15, 2015, 11:26 p.m.)
Review request for kafka.
Bugs: kafka-1952
https://issues.apache.org/jira/browse/kafka-1952
Repository: kafka
Description (updated)
-------
address review comments
Diffs (updated)
-----
core/src/main/scala/kafka/server/RequestPurgatory.scala 9d76234bc2c810ec08621dc92bb4061b8e7cd993
Diff: https://reviews.apache.org/r/31040/diff/
Testing
-------
Thanks,
Jun Rao
Re: Review Request 31040: Patch for kafka-1952
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31040/#review72479
-----------------------------------------------------------
Ship it!
core/src/main/scala/kafka/server/RequestPurgatory.scala
<https://reviews.apache.org/r/31040/#comment118541>
"<= 0)" ?
- Guozhang Wang
On Feb. 14, 2015, 1:52 a.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31040/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2015, 1:52 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1952
> https://issues.apache.org/jira/browse/kafka-1952
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1952; High CPU Usage in 0.8.2 release
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/RequestPurgatory.scala 9d76234bc2c810ec08621dc92bb4061b8e7cd993
>
> Diff: https://reviews.apache.org/r/31040/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 31040: Patch for kafka-1952
Posted by Jun Rao <ju...@gmail.com>.
> On Feb. 14, 2015, 5 a.m., Ewen Cheslack-Postava wrote:
> > core/src/main/scala/kafka/server/RequestPurgatory.scala, line 136
> > <https://reviews.apache.org/r/31040/diff/1/?file=864054#file864054line136>
> >
> > The
> >
> > if (t.satisfied.get)
> > return false
> >
> > part got removed when you converted checkAndMaybeAdd -> add. Is the assumption that adding the request for watch on all the keys is fast enough that it's unlikely to get satisfied in that time? If there is a decent chance, is it better to keep the request off more keys' watch lists by adding that check back in, or will it not have a substantial impact on the extra state this patch causes the purgatory to hold on to?
> >
> > I think the current code probably makes more sense, but I want to check since, given the CPU usage problem wasn't identified in the original patch it seems we previously thought the check was worth it.
That's a good point. Will add the extra check.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31040/#review72482
-----------------------------------------------------------
On Feb. 14, 2015, 1:52 a.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31040/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2015, 1:52 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1952
> https://issues.apache.org/jira/browse/kafka-1952
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1952; High CPU Usage in 0.8.2 release
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/RequestPurgatory.scala 9d76234bc2c810ec08621dc92bb4061b8e7cd993
>
> Diff: https://reviews.apache.org/r/31040/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 31040: Patch for kafka-1952
Posted by Ewen Cheslack-Postava <me...@ewencp.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31040/#review72482
-----------------------------------------------------------
Minor question about a check that got removed, but otherwise LGTM.
core/src/main/scala/kafka/server/RequestPurgatory.scala
<https://reviews.apache.org/r/31040/#comment118542>
The
if (t.satisfied.get)
return false
part got removed when you converted checkAndMaybeAdd -> add. Is the assumption that adding the request for watch on all the keys is fast enough that it's unlikely to get satisfied in that time? If there is a decent chance, is it better to keep the request off more keys' watch lists by adding that check back in, or will it not have a substantial impact on the extra state this patch causes the purgatory to hold on to?
I think the current code probably makes more sense, but I want to check since, given the CPU usage problem wasn't identified in the original patch it seems we previously thought the check was worth it.
- Ewen Cheslack-Postava
On Feb. 14, 2015, 1:52 a.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31040/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2015, 1:52 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1952
> https://issues.apache.org/jira/browse/kafka-1952
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1952; High CPU Usage in 0.8.2 release
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/RequestPurgatory.scala 9d76234bc2c810ec08621dc92bb4061b8e7cd993
>
> Diff: https://reviews.apache.org/r/31040/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>