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