You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Guozhang Wang <wa...@gmail.com> on 2015/06/08 06:46:26 UTC
Review Request 35201: Fix KAFKA-2253
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35201/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-2253
https://issues.apache.org/jira/browse/KAFKA-2253
Repository: kafka
Description
-------
Incorporated Jiangjie and Onur's comments
Diffs
-----
core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
Diff: https://reviews.apache.org/r/35201/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 35201: Fix KAFKA-2253
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35201/#review87065
-----------------------------------------------------------
Ship it!
Ship It!
- Jiangjie Qin
On June 8, 2015, 6:47 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35201/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 6:47 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2253
> https://issues.apache.org/jira/browse/KAFKA-2253
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Jiangjie and Onur's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
>
> Diff: https://reviews.apache.org/r/35201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 35201: Fix KAFKA-2253
Posted by Onur Karaman <ok...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35201/#review87077
-----------------------------------------------------------
Ship it!
Ship It!
- Onur Karaman
On June 8, 2015, 6:47 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35201/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 6:47 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2253
> https://issues.apache.org/jira/browse/KAFKA-2253
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Jiangjie and Onur's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
>
> Diff: https://reviews.apache.org/r/35201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 35201: Fix KAFKA-2253
Posted by Jiangjie Qin <be...@gmail.com>.
> On June 11, 2015, 1:07 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/DelayedOperation.scala, lines 264-266
> > <https://reviews.apache.org/r/35201/diff/2/?file=980805#file980805line264>
> >
> > Not sure if we need this check. Since all writes to watchersForKey are sync-ed, it's ok to remove a watcher as long as its count is 0.
> >
> > I am bit concerned about the overhead on the removeWatchersLock, which is global. For example, if you have 1000 requests/sec and each request has 1000 partitions, that lock is going to be access 1million times in a sec. Could you do some tests/profiling before and after we introduced the global lock to see if this could be an issue?
The lock is only grabbed when a watcher has no operations in it. But I agree that could be an issue.
I'm wondering is there a reason that we have to remove a watcher immediately when its count become zero? Can we just let the reaper remove empty watchers?
- Jiangjie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35201/#review87495
-----------------------------------------------------------
On June 8, 2015, 6:47 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35201/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 6:47 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2253
> https://issues.apache.org/jira/browse/KAFKA-2253
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Jiangjie and Onur's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
>
> Diff: https://reviews.apache.org/r/35201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 35201: Fix KAFKA-2253
Posted by Jun Rao <ju...@gmail.com>.
> On June 11, 2015, 1:07 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/DelayedOperation.scala, lines 264-266
> > <https://reviews.apache.org/r/35201/diff/2/?file=980805#file980805line264>
> >
> > Not sure if we need this check. Since all writes to watchersForKey are sync-ed, it's ok to remove a watcher as long as its count is 0.
> >
> > I am bit concerned about the overhead on the removeWatchersLock, which is global. For example, if you have 1000 requests/sec and each request has 1000 partitions, that lock is going to be access 1million times in a sec. Could you do some tests/profiling before and after we introduced the global lock to see if this could be an issue?
>
> Jiangjie Qin wrote:
> The lock is only grabbed when a watcher has no operations in it. But I agree that could be an issue.
> I'm wondering is there a reason that we have to remove a watcher immediately when its count become zero? Can we just let the reaper remove empty watchers?
>
> Onur Karaman wrote:
> +1 on letting the reaper do it.
Both tryCompleteElseWatch() and checkAndComplete() need to grab the global read lock. This is my main concern since this means every produce/fetch request pays a synchronization overhead on a single semaphore for every partition in the request. Some performance testing on the impact of this will be useful.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35201/#review87495
-----------------------------------------------------------
On June 8, 2015, 6:47 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35201/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 6:47 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2253
> https://issues.apache.org/jira/browse/KAFKA-2253
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Jiangjie and Onur's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
>
> Diff: https://reviews.apache.org/r/35201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 35201: Fix KAFKA-2253
Posted by Onur Karaman <ok...@linkedin.com>.
> On June 11, 2015, 1:07 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/DelayedOperation.scala, lines 264-266
> > <https://reviews.apache.org/r/35201/diff/2/?file=980805#file980805line264>
> >
> > Not sure if we need this check. Since all writes to watchersForKey are sync-ed, it's ok to remove a watcher as long as its count is 0.
> >
> > I am bit concerned about the overhead on the removeWatchersLock, which is global. For example, if you have 1000 requests/sec and each request has 1000 partitions, that lock is going to be access 1million times in a sec. Could you do some tests/profiling before and after we introduced the global lock to see if this could be an issue?
>
> Jiangjie Qin wrote:
> The lock is only grabbed when a watcher has no operations in it. But I agree that could be an issue.
> I'm wondering is there a reason that we have to remove a watcher immediately when its count become zero? Can we just let the reaper remove empty watchers?
+1 on letting the reaper do it.
- Onur
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35201/#review87495
-----------------------------------------------------------
On June 8, 2015, 6:47 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35201/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 6:47 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2253
> https://issues.apache.org/jira/browse/KAFKA-2253
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Jiangjie and Onur's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
>
> Diff: https://reviews.apache.org/r/35201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 35201: Fix KAFKA-2253
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35201/#review87495
-----------------------------------------------------------
core/src/main/scala/kafka/server/DelayedOperation.scala
<https://reviews.apache.org/r/35201/#comment139836>
Not sure if we need this check. Since all writes to watchersForKey are sync-ed, it's ok to remove a watcher as long as its count is 0.
I am bit concerned about the overhead on the removeWatchersLock, which is global. For example, if you have 1000 requests/sec and each request has 1000 partitions, that lock is going to be access 1million times in a sec. Could you do some tests/profiling before and after we introduced the global lock to see if this could be an issue?
- Jun Rao
On June 8, 2015, 6:47 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35201/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 6:47 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2253
> https://issues.apache.org/jira/browse/KAFKA-2253
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Jiangjie and Onur's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
>
> Diff: https://reviews.apache.org/r/35201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 35201: Fix KAFKA-2253
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35201/
-----------------------------------------------------------
(Updated June 8, 2015, 6:47 p.m.)
Review request for kafka.
Bugs: KAFKA-2253
https://issues.apache.org/jira/browse/KAFKA-2253
Repository: kafka
Description
-------
Incorporated Jiangjie and Onur's comments
Diffs (updated)
-----
core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
Diff: https://reviews.apache.org/r/35201/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 35201: Fix KAFKA-2253
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35201/#review86987
-----------------------------------------------------------
core/src/main/scala/kafka/server/DelayedOperation.scala
<https://reviews.apache.org/r/35201/#comment139198>
Will watchForOperation sound more precise?
core/src/main/scala/kafka/server/DelayedOperation.scala
<https://reviews.apache.org/r/35201/#comment139197>
Here we are grabbing removeWatchersLock first then synchronize on the watchers. In tryCompleteWatched, we synchronize on the watcher first then grab the removeWatchersLock. Will there be a deadlock?
- Jiangjie Qin
On June 8, 2015, 4:46 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35201/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 4:46 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2253
> https://issues.apache.org/jira/browse/KAFKA-2253
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Jiangjie and Onur's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/DelayedOperation.scala 123078d97a7bfe2121655c00f3b2c6af21c53015
>
> Diff: https://reviews.apache.org/r/35201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>