Re: Review Request 30570: Patch for KAFKA-1914

[Joel Koshy]

> On Feb. 18, 2015, 10:16 p.m., Jun Rao wrote:
> > Thanks for the patch. A couple of comments.
> > 
> > 1. We already measure the total produce/fetch request rate in 
> > RequestMetrics. Now, we are duplicating that in BrokerTopics. Is there a 
> > way to avoid the duplication?
> > 2. The following unit test fails consistently for me, when running all unit 
> > tests. It does pass when running individually.
> > 
> > kafka.server.SimpleFetchTest > testReadFromLog FAILED
> > junit.framework.AssertionFailedError: Counts should increment after 
> > fetch expected:<5> but was:<51432>
> > at junit.framework.Assert.fail(Assert.java:47)
> > at junit.framework.Assert.failNotEquals(Assert.java:277)
> > at junit.framework.Assert.assertEquals(Assert.java:64)
> > at junit.framework.Assert.assertEquals(Assert.java:130)
> > at 
> > kafka.server.SimpleFetchTest.testReadFromLog(SimpleFetchTest.scala:145)
> 
> Joel Koshy wrote:
> For (1) I noted this on the review but it is convenient to have the total 
> under BrokerTopicMetrics as well which is a reasonable tradeoff since it only 
> adds two additional meters.
> 
> For (2) Aditya, can you look into that?

BTW, this is because BrokerTopicMetrics is a singleton which is why the single 
run works. So I'm going to remove those checks for now.


- Joel


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review73020
---


On Feb. 17, 2015, 11:46 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 17, 2015, 11:46 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> BrokerTopicStats changes to aggregate on a per-topic basis the total fetch 
> and produce requests
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Re: Review Request 30570: Patch for KAFKA-1914

[Joel Koshy]

> On Feb. 18, 2015, 10:16 p.m., Jun Rao wrote:
> > Thanks for the patch. A couple of comments.
> > 
> > 1. We already measure the total produce/fetch request rate in 
> > RequestMetrics. Now, we are duplicating that in BrokerTopics. Is there a 
> > way to avoid the duplication?
> > 2. The following unit test fails consistently for me, when running all unit 
> > tests. It does pass when running individually.
> > 
> > kafka.server.SimpleFetchTest > testReadFromLog FAILED
> > junit.framework.AssertionFailedError: Counts should increment after 
> > fetch expected:<5> but was:<51432>
> > at junit.framework.Assert.fail(Assert.java:47)
> > at junit.framework.Assert.failNotEquals(Assert.java:277)
> > at junit.framework.Assert.assertEquals(Assert.java:64)
> > at junit.framework.Assert.assertEquals(Assert.java:130)
> > at 
> > kafka.server.SimpleFetchTest.testReadFromLog(SimpleFetchTest.scala:145)

For (1) I noted this on the review but it is convenient to have the total under 
BrokerTopicMetrics as well which is a reasonable tradeoff since it only adds 
two additional meters.

For (2) Aditya, can you look into that?


- Joel


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review73020
---


On Feb. 17, 2015, 11:46 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 17, 2015, 11:46 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> BrokerTopicStats changes to aggregate on a per-topic basis the total fetch 
> and produce requests
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Re: Review Request 30570: Patch for KAFKA-1914

[Jun Rao]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review73020
---


Thanks for the patch. A couple of comments.

1. We already measure the total produce/fetch request rate in RequestMetrics. 
Now, we are duplicating that in BrokerTopics. Is there a way to avoid the 
duplication?
2. The following unit test fails consistently for me, when running all unit 
tests. It does pass when running individually.

kafka.server.SimpleFetchTest > testReadFromLog FAILED
junit.framework.AssertionFailedError: Counts should increment after fetch 
expected:<5> but was:<51432>
at junit.framework.Assert.fail(Assert.java:47)
at junit.framework.Assert.failNotEquals(Assert.java:277)
at junit.framework.Assert.assertEquals(Assert.java:64)
at junit.framework.Assert.assertEquals(Assert.java:130)
at 
kafka.server.SimpleFetchTest.testReadFromLog(SimpleFetchTest.scala:145)

- Jun Rao


On Feb. 17, 2015, 11:46 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 17, 2015, 11:46 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> BrokerTopicStats changes to aggregate on a per-topic basis the total fetch 
> and produce requests
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Re: Review Request 30570: Patch for KAFKA-1914

[Aditya Auradkar]

> On Feb. 18, 2015, 12:41 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/KafkaRequestHandler.scala, line 108
> > 
> >
> > I think the aggregate rates here are redundant to what's already there 
> > in RequestChannel's request metrics; but I think it is convenient to have 
> > it here as well.

Thanks! But do the ones in RequestChannel aggregate on a per-topic basis?


- Aditya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review72845
---


On Feb. 17, 2015, 11:46 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 17, 2015, 11:46 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> BrokerTopicStats changes to aggregate on a per-topic basis the total fetch 
> and produce requests
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Re: Review Request 30570: Patch for KAFKA-1914

[Joel Koshy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review72845
---

Ship it!



core/src/main/scala/kafka/server/KafkaRequestHandler.scala


I think the aggregate rates here are redundant to what's already there in 
RequestChannel's request metrics; but I think it is convenient to have it here 
as well.


- Joel Koshy


On Feb. 17, 2015, 11:46 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 17, 2015, 11:46 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> BrokerTopicStats changes to aggregate on a per-topic basis the total fetch 
> and produce requests
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Re: Review Request 30570: Patch for KAFKA-1914

[Guozhang Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review72836
---

Ship it!


Ship It!

- Guozhang Wang


On Feb. 17, 2015, 11:46 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 17, 2015, 11:46 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> BrokerTopicStats changes to aggregate on a per-topic basis the total fetch 
> and produce requests
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Re: Review Request 30570: Patch for KAFKA-1914

[Aditya Auradkar]

> On Feb. 17, 2015, 11:02 p.m., Guozhang Wang wrote:
> > core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala, line 137
> > 
> >
> > Should this be BrokerTopicStats.getBrokerAllTopicsStats()?

Good catch. Fixed


- Aditya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review72817
---


On Feb. 17, 2015, 11:46 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 17, 2015, 11:46 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> BrokerTopicStats changes to aggregate on a per-topic basis the total fetch 
> and produce requests
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Re: Review Request 30570: Patch for KAFKA-1914

[Aditya Auradkar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/
---

(Updated Feb. 17, 2015, 11:46 p.m.)


Review request for kafka and Joel Koshy.


Bugs: KAFKA-1914
https://issues.apache.org/jira/browse/KAFKA-1914


Repository: kafka


Description (updated)
---

BrokerTopicStats changes to aggregate on a per-topic basis the total fetch and 
produce requests


Diffs (updated)
-

  core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
  core/src/main/scala/kafka/server/ReplicaManager.scala 
fb948b9ab28c516e81dab14dcbe211dcd99842b6 
  core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
ccf5e2e36260b2484181b81d1b06e81de972674b 

Diff: https://reviews.apache.org/r/30570/diff/


Testing
---

I've added asserts to the SimpleFetchTest to count the number of fetch 
requests. I'm going to file an additional jira to add unit tests for all the 
BrokerTopicMetrics updated via ReplicaManager


Thanks,

Aditya Auradkar

Re: Review Request 30570: Patch for KAFKA-1914

[Guozhang Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review72817
---



core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala


Should this be BrokerTopicStats.getBrokerAllTopicsStats()?


- Guozhang Wang


On Feb. 3, 2015, 7:13 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 3, 2015, 7:13 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Fixing KAFKA-1914. Adding metrics to count total number of produce and fetch 
> metrics
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Re: Review Request 30570: Patch for KAFKA-1914

[Aditya Auradkar]

> On Feb. 9, 2015, 6:29 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, lines 296-303
> > 
> >
> > I agree with Guozhang that the offset commits need to be disambiguated.
> > 
> > How about we do the following:
> > * Aditya, you can go ahead with this change. i.e., record the total 
> > rate in ReplicaManager
> > * File a separate jira to address the discrepancies. E.g., right now 
> > offset commit failures due to append failures are counted in failed produce 
> > rate.

Thanks Joel! I've filed this ticket 
https://issues.apache.org/jira/browse/KAFKA-1936.


- Aditya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review71660
---


On Feb. 3, 2015, 7:13 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 3, 2015, 7:13 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Fixing KAFKA-1914. Adding metrics to count total number of produce and fetch 
> metrics
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Re: Review Request 30570: Patch for KAFKA-1914

[Joel Koshy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review71660
---



core/src/main/scala/kafka/server/ReplicaManager.scala


I agree with Guozhang that the offset commits need to be disambiguated.

How about we do the following:
* Aditya, you can go ahead with this change. i.e., record the total rate in 
ReplicaManager
* File a separate jira to address the discrepancies. E.g., right now offset 
commit failures due to append failures are counted in failed produce rate.


- Joel Koshy


On Feb. 3, 2015, 7:13 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 3, 2015, 7:13 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Fixing KAFKA-1914. Adding metrics to count total number of produce and fetch 
> metrics
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Re: Review Request 30570: Patch for KAFKA-1914

[Aditya Auradkar]

> On Feb. 9, 2015, 5:47 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, lines 296-303
> > 
> >
> > appendToLocalLog can be called for both produce and commit-offset 
> > requests. And in general I think we might better record the request-level 
> > metrics on KafkaApis layer intead of the ReplicaManager layer. There are 
> > some previous work on isolating the request-level information in KafkaApis 
> > layer.
> 
> Aditya Auradkar wrote:
> All the BrokerTopicMetrics reporting happens below KafkaApis i.e. inside 
> the ReplicaManager and Log layer. Since failedProduceRequestRate is being 
> counted in appendToLocalLog, I felt it was more consistent to report 
> totalProduceRequestRate also in the same place. If we do want to report these 
> new stats in KafkaApis, then we should probably change existing metrics 
> reporting as well. bytesInRate, bytesOutRate, failedProduceRequestRate, 
> failedFetchRequestRate etc.. In general it does seem nicer to track these 
> metrics in KafkaApis and I can certainly work on this but IMO it should be 
> tracked separately from this issue. Thoughts?
> 
> Regarding offset-commit, aren't the new commit-offset requests simply 
> produce requests to a special topic? In that case, it doesn't seem 
> appropriate to do this here.

Typo: 
"Regarding offset-commit, aren't the new commit-offset requests simply produce 
requests to a special topic? In that case, it doesn't seem inappropriate to do 
this here."


- Aditya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review71646
---


On Feb. 3, 2015, 7:13 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 3, 2015, 7:13 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Fixing KAFKA-1914. Adding metrics to count total number of produce and fetch 
> metrics
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Re: Review Request 30570: Patch for KAFKA-1914

[Aditya Auradkar]

> On Feb. 9, 2015, 5:47 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/server/ReplicaManager.scala, lines 296-303
> > 
> >
> > appendToLocalLog can be called for both produce and commit-offset 
> > requests. And in general I think we might better record the request-level 
> > metrics on KafkaApis layer intead of the ReplicaManager layer. There are 
> > some previous work on isolating the request-level information in KafkaApis 
> > layer.

All the BrokerTopicMetrics reporting happens below KafkaApis i.e. inside the 
ReplicaManager and Log layer. Since failedProduceRequestRate is being counted 
in appendToLocalLog, I felt it was more consistent to report 
totalProduceRequestRate also in the same place. If we do want to report these 
new stats in KafkaApis, then we should probably change existing metrics 
reporting as well. bytesInRate, bytesOutRate, failedProduceRequestRate, 
failedFetchRequestRate etc.. In general it does seem nicer to track these 
metrics in KafkaApis and I can certainly work on this but IMO it should be 
tracked separately from this issue. Thoughts?

Regarding offset-commit, aren't the new commit-offset requests simply produce 
requests to a special topic? In that case, it doesn't seem appropriate to do 
this here.


- Aditya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review71646
---


On Feb. 3, 2015, 7:13 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 3, 2015, 7:13 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Fixing KAFKA-1914. Adding metrics to count total number of produce and fetch 
> metrics
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Re: Review Request 30570: Patch for KAFKA-1914

[Guozhang Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/#review71646
---



core/src/main/scala/kafka/server/ReplicaManager.scala


appendToLocalLog can be called for both produce and commit-offset requests. 
And in general I think we might better record the request-level metrics on 
KafkaApis layer intead of the ReplicaManager layer. There are some previous 
work on isolating the request-level information in KafkaApis layer.


- Guozhang Wang


On Feb. 3, 2015, 7:13 p.m., Aditya Auradkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30570/
> ---
> 
> (Updated Feb. 3, 2015, 7:13 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-1914
> https://issues.apache.org/jira/browse/KAFKA-1914
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Fixing KAFKA-1914. Adding metrics to count total number of produce and fetch 
> metrics
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
> e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
>   core/src/main/scala/kafka/server/ReplicaManager.scala 
> fb948b9ab28c516e81dab14dcbe211dcd99842b6 
>   core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
> ccf5e2e36260b2484181b81d1b06e81de972674b 
> 
> Diff: https://reviews.apache.org/r/30570/diff/
> 
> 
> Testing
> ---
> 
> I've added asserts to the SimpleFetchTest to count the number of fetch 
> requests. I'm going to file an additional jira to add unit tests for all the 
> BrokerTopicMetrics updated via ReplicaManager
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>

Review Request 30570: Patch for KAFKA-1914

[Aditya Auradkar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/
---

Review request for kafka.


Bugs: KAFKA-1914
https://issues.apache.org/jira/browse/KAFKA-1914


Repository: kafka


Description
---

Fixing KAFKA-1914. Adding metrics to count total number of produce and fetch 
metrics


Diffs
-

  core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
  core/src/main/scala/kafka/server/ReplicaManager.scala 
fb948b9ab28c516e81dab14dcbe211dcd99842b6 
  core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
ccf5e2e36260b2484181b81d1b06e81de972674b 

Diff: https://reviews.apache.org/r/30570/diff/


Testing
---


Thanks,

Aditya Auradkar

Re: Review Request 30570: Patch for KAFKA-1914

[Aditya Auradkar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30570/
---

(Updated Feb. 3, 2015, 6:50 p.m.)


Review request for kafka.


Bugs: KAFKA-1914
https://issues.apache.org/jira/browse/KAFKA-1914


Repository: kafka


Description
---

Fixing KAFKA-1914. Adding metrics to count total number of produce and fetch 
metrics


Diffs
-

  core/src/main/scala/kafka/server/KafkaRequestHandler.scala 
e4053fbe8ef78bf8bc39cb3f8ea4c21032613a16 
  core/src/main/scala/kafka/server/ReplicaManager.scala 
fb948b9ab28c516e81dab14dcbe211dcd99842b6 
  core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 
ccf5e2e36260b2484181b81d1b06e81de972674b 

Diff: https://reviews.apache.org/r/30570/diff/


Testing (updated)
---

I've added asserts to the SimpleFetchTest to count the number of fetch 
requests. I'm going to file an additional jira to add unit tests for all the 
BrokerTopicMetrics updated via ReplicaManager


Thanks,

Aditya Auradkar