You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Joel Koshy <jj...@gmail.com> on 2015/02/18 01:48:20 UTC
Review Request 31140: Patch for KAFKA-1953
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31140/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1953
https://issues.apache.org/jira/browse/KAFKA-1953
Repository: kafka
Description
-------
KAFKA-1953; KAFKA-1962; Disambiguate purgatory metrics; restore delayed request metrics
Diffs
-----
core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala 01cf1d91b7056bea7368ae4ea1e3c3646fc33619
core/src/main/scala/kafka/coordinator/DelayedHeartbeat.scala 894d6edb4077cae081b9d4039353dd17e6f0c18f
core/src/main/scala/kafka/coordinator/DelayedJoinGroup.scala 445bfa1bf8840620e10de2456875716dc66e789a
core/src/main/scala/kafka/coordinator/DelayedRebalance.scala b3b3749a21d35950a975e24dd9d1d53afbfaaee4
core/src/main/scala/kafka/server/DelayedFetch.scala dd602ee2e65c2cd4ec363c75fa5d0b3c038b1ed2
core/src/main/scala/kafka/server/DelayedOperation.scala fc06b01cad3a0497800df727fa2abf60772694f2
core/src/main/scala/kafka/server/DelayedProduce.scala c229088eb4f3db414225a688e149591ae0f810e7
core/src/main/scala/kafka/server/ReplicaManager.scala ce36cc72606fb5441335f1c7466a7db8da3db499
core/src/test/scala/unit/kafka/server/DelayedOperationTest.scala 93f52d3222fc10b6d22ef6278365f6b026180418
Diff: https://reviews.apache.org/r/31140/diff/
Testing
-------
Thanks,
Joel Koshy
Re: Review Request 31140: Patch for KAFKA-1953
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31140/#review72849
-----------------------------------------------------------
Couple of comments to call out.
core/src/main/scala/kafka/server/DelayedOperation.scala
<https://reviews.apache.org/r/31140/#comment118961>
FYI, I tried to avoid doing this. i.e., providing an explicit name.
We cannot simply do T.getClass.getName but have to look up manifests: http://stackoverflow.com/questions/8208179/scala-obtaining-a-class-object-from-a-generic-type
The previous code avoided this by extending this class.
E.g., class ProducerRequestPurgatory extends DelayedOperationPurgatory...
I actually prefer the explicit name to that approach because it forces people to think about the name. Otherwise if people forget to extend for multiple purgatory instances then the metrics could collide.
core/src/main/scala/kafka/server/DelayedProduce.scala
<https://reviews.apache.org/r/31140/#comment118962>
Note that this is similar to our expiration recording in the past, although it is slightly weird. i.e., each expired key counts toward the aggregate even if it is all from one single producer request.
- Joel Koshy
On Feb. 18, 2015, 12:48 a.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31140/
> -----------------------------------------------------------
>
> (Updated Feb. 18, 2015, 12:48 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1953
> https://issues.apache.org/jira/browse/KAFKA-1953
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1953; KAFKA-1962; Disambiguate purgatory metrics; restore delayed request metrics
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala 01cf1d91b7056bea7368ae4ea1e3c3646fc33619
> core/src/main/scala/kafka/coordinator/DelayedHeartbeat.scala 894d6edb4077cae081b9d4039353dd17e6f0c18f
> core/src/main/scala/kafka/coordinator/DelayedJoinGroup.scala 445bfa1bf8840620e10de2456875716dc66e789a
> core/src/main/scala/kafka/coordinator/DelayedRebalance.scala b3b3749a21d35950a975e24dd9d1d53afbfaaee4
> core/src/main/scala/kafka/server/DelayedFetch.scala dd602ee2e65c2cd4ec363c75fa5d0b3c038b1ed2
> core/src/main/scala/kafka/server/DelayedOperation.scala fc06b01cad3a0497800df727fa2abf60772694f2
> core/src/main/scala/kafka/server/DelayedProduce.scala c229088eb4f3db414225a688e149591ae0f810e7
> core/src/main/scala/kafka/server/ReplicaManager.scala ce36cc72606fb5441335f1c7466a7db8da3db499
> core/src/test/scala/unit/kafka/server/DelayedOperationTest.scala 93f52d3222fc10b6d22ef6278365f6b026180418
>
> Diff: https://reviews.apache.org/r/31140/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31140: Patch for KAFKA-1953
Posted by Joel Koshy <jj...@gmail.com>.
> On Feb. 18, 2015, 2:06 a.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/DelayedOperation.scala, line 286
> > <https://reviews.apache.org/r/31140/diff/1/?file=866729#file866729line286>
> >
> > We can move the debug statement out of the synchronized block.
Good point.
> On Feb. 18, 2015, 2:06 a.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/DelayedProduce.scala, line 147
> > <https://reviews.apache.org/r/31140/diff/1/?file=866730#file866730line147>
> >
> > Were topic/partition expiration metrics useful in the past?
Yes, I can see those as being useful. However, it is likely not one of those critical metrics that you would want on a dashboard. We can discuss whether we want to keep those or not.
> On Feb. 18, 2015, 2:06 a.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, lines 85-88
> > <https://reviews.apache.org/r/31140/diff/1/?file=866731#file866731line85>
> >
> > We can remove "purgatoryName = " here.
When passing in literals it is best to provide named parameters otherwise it is not always clear from the call itself what the parameter actually means.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31140/#review72875
-----------------------------------------------------------
On Feb. 18, 2015, 12:48 a.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31140/
> -----------------------------------------------------------
>
> (Updated Feb. 18, 2015, 12:48 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1953
> https://issues.apache.org/jira/browse/KAFKA-1953
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1953; KAFKA-1962; Disambiguate purgatory metrics; restore delayed request metrics
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala 01cf1d91b7056bea7368ae4ea1e3c3646fc33619
> core/src/main/scala/kafka/coordinator/DelayedHeartbeat.scala 894d6edb4077cae081b9d4039353dd17e6f0c18f
> core/src/main/scala/kafka/coordinator/DelayedJoinGroup.scala 445bfa1bf8840620e10de2456875716dc66e789a
> core/src/main/scala/kafka/coordinator/DelayedRebalance.scala b3b3749a21d35950a975e24dd9d1d53afbfaaee4
> core/src/main/scala/kafka/server/DelayedFetch.scala dd602ee2e65c2cd4ec363c75fa5d0b3c038b1ed2
> core/src/main/scala/kafka/server/DelayedOperation.scala fc06b01cad3a0497800df727fa2abf60772694f2
> core/src/main/scala/kafka/server/DelayedProduce.scala c229088eb4f3db414225a688e149591ae0f810e7
> core/src/main/scala/kafka/server/ReplicaManager.scala ce36cc72606fb5441335f1c7466a7db8da3db499
> core/src/test/scala/unit/kafka/server/DelayedOperationTest.scala 93f52d3222fc10b6d22ef6278365f6b026180418
>
> Diff: https://reviews.apache.org/r/31140/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31140: Patch for KAFKA-1953
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31140/#review72875
-----------------------------------------------------------
core/src/main/scala/kafka/server/DelayedOperation.scala
<https://reviews.apache.org/r/31140/#comment119000>
We can move the debug statement out of the synchronized block.
core/src/main/scala/kafka/server/DelayedProduce.scala
<https://reviews.apache.org/r/31140/#comment118996>
Were topic/partition expiration metrics useful in the past?
core/src/main/scala/kafka/server/ReplicaManager.scala
<https://reviews.apache.org/r/31140/#comment118998>
We can remove "purgatoryName = " here.
- Guozhang Wang
On Feb. 18, 2015, 12:48 a.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31140/
> -----------------------------------------------------------
>
> (Updated Feb. 18, 2015, 12:48 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1953
> https://issues.apache.org/jira/browse/KAFKA-1953
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1953; KAFKA-1962; Disambiguate purgatory metrics; restore delayed request metrics
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala 01cf1d91b7056bea7368ae4ea1e3c3646fc33619
> core/src/main/scala/kafka/coordinator/DelayedHeartbeat.scala 894d6edb4077cae081b9d4039353dd17e6f0c18f
> core/src/main/scala/kafka/coordinator/DelayedJoinGroup.scala 445bfa1bf8840620e10de2456875716dc66e789a
> core/src/main/scala/kafka/coordinator/DelayedRebalance.scala b3b3749a21d35950a975e24dd9d1d53afbfaaee4
> core/src/main/scala/kafka/server/DelayedFetch.scala dd602ee2e65c2cd4ec363c75fa5d0b3c038b1ed2
> core/src/main/scala/kafka/server/DelayedOperation.scala fc06b01cad3a0497800df727fa2abf60772694f2
> core/src/main/scala/kafka/server/DelayedProduce.scala c229088eb4f3db414225a688e149591ae0f810e7
> core/src/main/scala/kafka/server/ReplicaManager.scala ce36cc72606fb5441335f1c7466a7db8da3db499
> core/src/test/scala/unit/kafka/server/DelayedOperationTest.scala 93f52d3222fc10b6d22ef6278365f6b026180418
>
> Diff: https://reviews.apache.org/r/31140/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31140: Patch for KAFKA-1953
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31140/#review73176
-----------------------------------------------------------
Ship it!
Ship It!
- Guozhang Wang
On Feb. 18, 2015, 2:23 a.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31140/
> -----------------------------------------------------------
>
> (Updated Feb. 18, 2015, 2:23 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1953
> https://issues.apache.org/jira/browse/KAFKA-1953
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1953; KAFKA-1962; Disambiguate purgatory metrics; restore delayed request metrics
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala 01cf1d91b7056bea7368ae4ea1e3c3646fc33619
> core/src/main/scala/kafka/coordinator/DelayedHeartbeat.scala 894d6edb4077cae081b9d4039353dd17e6f0c18f
> core/src/main/scala/kafka/coordinator/DelayedJoinGroup.scala 445bfa1bf8840620e10de2456875716dc66e789a
> core/src/main/scala/kafka/coordinator/DelayedRebalance.scala b3b3749a21d35950a975e24dd9d1d53afbfaaee4
> core/src/main/scala/kafka/server/DelayedFetch.scala dd602ee2e65c2cd4ec363c75fa5d0b3c038b1ed2
> core/src/main/scala/kafka/server/DelayedOperation.scala fc06b01cad3a0497800df727fa2abf60772694f2
> core/src/main/scala/kafka/server/DelayedProduce.scala c229088eb4f3db414225a688e149591ae0f810e7
> core/src/main/scala/kafka/server/ReplicaManager.scala b82ff55e1dd1fe3fee2de5ab4bbddc91b0146601
> core/src/test/scala/unit/kafka/server/DelayedOperationTest.scala 93f52d3222fc10b6d22ef6278365f6b026180418
>
> Diff: https://reviews.apache.org/r/31140/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31140: Patch for KAFKA-1953
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31140/
-----------------------------------------------------------
(Updated Feb. 18, 2015, 2:23 a.m.)
Review request for kafka.
Bugs: KAFKA-1953
https://issues.apache.org/jira/browse/KAFKA-1953
Repository: kafka
Description
-------
KAFKA-1953; KAFKA-1962; Disambiguate purgatory metrics; restore delayed request metrics
Diffs (updated)
-----
core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala 01cf1d91b7056bea7368ae4ea1e3c3646fc33619
core/src/main/scala/kafka/coordinator/DelayedHeartbeat.scala 894d6edb4077cae081b9d4039353dd17e6f0c18f
core/src/main/scala/kafka/coordinator/DelayedJoinGroup.scala 445bfa1bf8840620e10de2456875716dc66e789a
core/src/main/scala/kafka/coordinator/DelayedRebalance.scala b3b3749a21d35950a975e24dd9d1d53afbfaaee4
core/src/main/scala/kafka/server/DelayedFetch.scala dd602ee2e65c2cd4ec363c75fa5d0b3c038b1ed2
core/src/main/scala/kafka/server/DelayedOperation.scala fc06b01cad3a0497800df727fa2abf60772694f2
core/src/main/scala/kafka/server/DelayedProduce.scala c229088eb4f3db414225a688e149591ae0f810e7
core/src/main/scala/kafka/server/ReplicaManager.scala b82ff55e1dd1fe3fee2de5ab4bbddc91b0146601
core/src/test/scala/unit/kafka/server/DelayedOperationTest.scala 93f52d3222fc10b6d22ef6278365f6b026180418
Diff: https://reviews.apache.org/r/31140/diff/
Testing
-------
Thanks,
Joel Koshy