You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Yasuhiro Matsuda <ya...@gmail.com> on 2015/03/10 17:41:55 UTC
Review Request 31893: Patch for KAFKA-2013
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31893/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-2013
https://issues.apache.org/jira/browse/KAFKA-2013
Repository: kafka
Description
-------
purgatory micro benchmark
Diffs
-----
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala PRE-CREATION
Diff: https://reviews.apache.org/r/31893/diff/
Testing
-------
Thanks,
Yasuhiro Matsuda
Re: Review Request 31893: Patch for KAFKA-2013
Posted by Yasuhiro Matsuda <ya...@gmail.com>.
> On March 31, 2015, 11:47 p.m., Jun Rao wrote:
> > core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala, line 261
> > <https://reviews.apache.org/r/31893/diff/5/?file=900766#file900766line261>
> >
> > Could we just put FakeOperation to the delayQueue directly instead of wrapping it under Scheduled?
We have to use completion time instead of expiration time for CompletionQueue. That is why the operaion is wrapped.
- Yasuhiro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31893/#review76975
-----------------------------------------------------------
On April 1, 2015, 12:31 a.m., Yasuhiro Matsuda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31893/
> -----------------------------------------------------------
>
> (Updated April 1, 2015, 12:31 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2013
> https://issues.apache.org/jira/browse/KAFKA-2013
>
>
> Repository: kafka
>
>
> Description
> -------
>
> purgatory micro benchmark
>
>
> Diffs
> -----
>
> core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31893/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Yasuhiro Matsuda
>
>
Re: Review Request 31893: Patch for KAFKA-2013
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31893/#review76975
-----------------------------------------------------------
Thanks for the new patch. I really like the desgin of this test! A few minor comments below.
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment127091>
The Try syntax doesn't seem to be supported in scala 2.9.
./gradlew -PscalaVersion=2.9.1 testJar
kafka/core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala:29: Try is not a member of scala.util
import scala.util.Try
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment127122>
Could we note that all times are in millisecs?
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment127085>
why => way
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment127086>
Could you add a comment on what 0.674490d stands for?
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment127110>
Now that we are completing the request in CompleteQueue explicitly, do we still need to override isCompleted()?
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment127104>
We probably can make this a ShutdownableThread.
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment127102>
Do we need the while loop? Could we just do a if test and go back to the outer loop?
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment127105>
Would it be more reliable to call forceComplete() here? Not sure if delayQueue always returns an item at the precise time.
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment127107>
Could we just put FakeOperation to the delayQueue directly instead of wrapping it under Scheduled?
- Jun Rao
On March 19, 2015, 11:31 p.m., Yasuhiro Matsuda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31893/
> -----------------------------------------------------------
>
> (Updated March 19, 2015, 11:31 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2013
> https://issues.apache.org/jira/browse/KAFKA-2013
>
>
> Repository: kafka
>
>
> Description
> -------
>
> purgatory micro benchmark
>
>
> Diffs
> -----
>
> core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31893/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Yasuhiro Matsuda
>
>
Re: Review Request 31893: Patch for KAFKA-2013
Posted by Yasuhiro Matsuda <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31893/
-----------------------------------------------------------
(Updated April 1, 2015, 12:31 a.m.)
Review request for kafka.
Bugs: KAFKA-2013
https://issues.apache.org/jira/browse/KAFKA-2013
Repository: kafka
Description
-------
purgatory micro benchmark
Diffs (updated)
-----
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala PRE-CREATION
Diff: https://reviews.apache.org/r/31893/diff/
Testing
-------
Thanks,
Yasuhiro Matsuda
Re: Review Request 31893: Patch for KAFKA-2013
Posted by Yasuhiro Matsuda <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31893/
-----------------------------------------------------------
(Updated March 19, 2015, 11:31 p.m.)
Review request for kafka.
Bugs: KAFKA-2013
https://issues.apache.org/jira/browse/KAFKA-2013
Repository: kafka
Description
-------
purgatory micro benchmark
Diffs (updated)
-----
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala PRE-CREATION
Diff: https://reviews.apache.org/r/31893/diff/
Testing
-------
Thanks,
Yasuhiro Matsuda
Re: Review Request 31893: Patch for KAFKA-2013
Posted by Yasuhiro Matsuda <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31893/
-----------------------------------------------------------
(Updated March 16, 2015, 9:39 p.m.)
Review request for kafka.
Bugs: KAFKA-2013
https://issues.apache.org/jira/browse/KAFKA-2013
Repository: kafka
Description
-------
purgatory micro benchmark
Diffs (updated)
-----
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala PRE-CREATION
Diff: https://reviews.apache.org/r/31893/diff/
Testing
-------
Thanks,
Yasuhiro Matsuda
Re: Review Request 31893: Patch for KAFKA-2013
Posted by Yasuhiro Matsuda <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31893/
-----------------------------------------------------------
(Updated March 16, 2015, 9:13 p.m.)
Review request for kafka.
Bugs: KAFKA-2013
https://issues.apache.org/jira/browse/KAFKA-2013
Repository: kafka
Description
-------
purgatory micro benchmark
Diffs (updated)
-----
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala PRE-CREATION
Diff: https://reviews.apache.org/r/31893/diff/
Testing
-------
Thanks,
Yasuhiro Matsuda
Re: Review Request 31893: Patch for KAFKA-2013
Posted by Yasuhiro Matsuda <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31893/
-----------------------------------------------------------
(Updated March 16, 2015, 8:23 p.m.)
Review request for kafka.
Bugs: KAFKA-2013
https://issues.apache.org/jira/browse/KAFKA-2013
Repository: kafka
Description
-------
purgatory micro benchmark
Diffs (updated)
-----
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala PRE-CREATION
Diff: https://reviews.apache.org/r/31893/diff/
Testing
-------
Thanks,
Yasuhiro Matsuda
Re: Review Request 31893: Patch for KAFKA-2013
Posted by Yasuhiro Matsuda <ya...@gmail.com>.
> On March 16, 2015, 5:17 p.m., Jun Rao wrote:
> > core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala, line 193
> > <https://reviews.apache.org/r/31893/diff/1/?file=890190#file890190line193>
> >
> > Is there a particular reason that we need to overwrite isCompleted()? Typically, only tryComplete() and onComplete() need to be overwritten in a subclass of DelayedOperation.
> >
> > Actually, I am not sure how we complete the requests before the timeout is reached since there is no explict call for tryComplete()?
isCompleted checks if the current time has passed the schedule completion time rather than if forceComplete has been called. It makes isCompleted always accurate.
Purgatory checks watcher lists every so often and calls isCompleted. Calling forceComplete from isCompeleted ensures that a completed request is removed from the timing wheels in the new implementation. In terms of timing, this is not very accurate because completed requests may stay longer then they should be. This doesn't affect the old implementaion at all, but it may impose some overheads on the new implementaion. Still, the new one outperforms the old one.
It is ideal if we can call call forceComplete on scheduled completion time. It requires another timer (DelayQueue or Timer) for that. I think it is too much overhead to measure purgatory performace. And also it is hard to guarantee such a timer works accurately in this test setting.
- Yasuhiro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31893/#review76566
-----------------------------------------------------------
On March 16, 2015, 8:23 p.m., Yasuhiro Matsuda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31893/
> -----------------------------------------------------------
>
> (Updated March 16, 2015, 8:23 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2013
> https://issues.apache.org/jira/browse/KAFKA-2013
>
>
> Repository: kafka
>
>
> Description
> -------
>
> purgatory micro benchmark
>
>
> Diffs
> -----
>
> core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31893/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Yasuhiro Matsuda
>
>
Re: Review Request 31893: Patch for KAFKA-2013
Posted by Yasuhiro Matsuda <ya...@gmail.com>.
> On March 16, 2015, 5:17 p.m., Jun Rao wrote:
> > core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala, line 193
> > <https://reviews.apache.org/r/31893/diff/1/?file=890190#file890190line193>
> >
> > Is there a particular reason that we need to overwrite isCompleted()? Typically, only tryComplete() and onComplete() need to be overwritten in a subclass of DelayedOperation.
> >
> > Actually, I am not sure how we complete the requests before the timeout is reached since there is no explict call for tryComplete()?
>
> Yasuhiro Matsuda wrote:
> isCompleted checks if the current time has passed the schedule completion time rather than if forceComplete has been called. It makes isCompleted always accurate.
>
> Purgatory checks watcher lists every so often and calls isCompleted. Calling forceComplete from isCompeleted ensures that a completed request is removed from the timing wheels in the new implementation. In terms of timing, this is not very accurate because completed requests may stay longer then they should be. This doesn't affect the old implementaion at all, but it may impose some overheads on the new implementaion. Still, the new one outperforms the old one.
>
> It is ideal if we can call call forceComplete on scheduled completion time. It requires another timer (DelayQueue or Timer) for that. I think it is too much overhead to measure purgatory performace. And also it is hard to guarantee such a timer works accurately in this test setting.
It looks the watcher list check happens frequent enough in both new and old implementations. The average delay to acutal forceComplete call from the completion time is several tens of millisecs (low request rate) to sub-millisecs (high request rate).
- Yasuhiro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31893/#review76566
-----------------------------------------------------------
On March 16, 2015, 9:39 p.m., Yasuhiro Matsuda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31893/
> -----------------------------------------------------------
>
> (Updated March 16, 2015, 9:39 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2013
> https://issues.apache.org/jira/browse/KAFKA-2013
>
>
> Repository: kafka
>
>
> Description
> -------
>
> purgatory micro benchmark
>
>
> Diffs
> -----
>
> core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31893/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Yasuhiro Matsuda
>
>
Re: Review Request 31893: Patch for KAFKA-2013
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31893/#review76566
-----------------------------------------------------------
Thanks for the patch. A few comments.
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment124137>
Do we need to make end volatile since it's being updated in separate thread?
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment124141>
Would it be better to rename this to sth like latencyToCompelete?
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment124142>
Variable due doesn't seem to be used?
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment124147>
I guess the sleep will be added when the actual rate exceeds the target rate? Would it be better to rename qtime as requestArrivalTime and interval as requestArrivalInterval?
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment124139>
It would be useful to make the # of keys configurable.
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment124138>
So far, we haven't used this syntax for println. For consistency, perhaps it's better to use the existing way of string formatting.
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment124143>
Could we add some comments on the meaning of mu and sigma?
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment124144>
Could we add some comments for the class? In particular, what does lamda mean?
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment124145>
It would be helpful to provide a high level description of what kind of distribution we get in the samples. Also, is there a particular reason that we pick LogNormal distribution instead of just normal distribution?
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment124150>
Could we add a bit of comment on how the sampling works? I guess it tries to spread the # requests into a 1000ms interval and returns the gap for the next request on every next() call?
Also, is there a particular reason that we want to choose exponential distribution to spread those requests instead of a simple uniform distribution (as done in ProducerPerformance)?
core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala
<https://reviews.apache.org/r/31893/#comment124148>
Is there a particular reason that we need to overwrite isCompleted()? Typically, only tryComplete() and onComplete() need to be overwritten in a subclass of DelayedOperation.
Actually, I am not sure how we complete the requests before the timeout is reached since there is no explict call for tryComplete()?
- Jun Rao
On March 10, 2015, 4:41 p.m., Yasuhiro Matsuda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31893/
> -----------------------------------------------------------
>
> (Updated March 10, 2015, 4:41 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2013
> https://issues.apache.org/jira/browse/KAFKA-2013
>
>
> Repository: kafka
>
>
> Description
> -------
>
> purgatory micro benchmark
>
>
> Diffs
> -----
>
> core/src/test/scala/other/kafka/TestPurgatoryPerformance.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31893/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Yasuhiro Matsuda
>
>