Re: Review Request 30570: Patch for KAFKA-1914

2015-02-18 Thread 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

2015-02-18 Thread 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

2015-02-17 Thread Aditya Auradkar


 On Feb. 17, 2015, 11:02 p.m., Guozhang Wang wrote:
  core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala, line 137
  https://reviews.apache.org/r/30570/diff/1/?file=846133#file846133line137
 
  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

2015-02-17 Thread 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

2015-02-17 Thread Aditya Auradkar


 On Feb. 18, 2015, 12:41 a.m., Joel Koshy wrote:
  core/src/main/scala/kafka/server/KafkaRequestHandler.scala, line 108
  https://reviews.apache.org/r/30570/diff/2/?file=866690#file866690line108
 
  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

2015-02-17 Thread 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
https://reviews.apache.org/r/30570/#comment118952

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

2015-02-17 Thread 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
https://reviews.apache.org/r/30570/#comment118901

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

2015-02-17 Thread 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

2015-02-09 Thread 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
https://reviews.apache.org/r/30570/#comment117510

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
 




Re: Review Request 30570: Patch for KAFKA-1914

2015-02-09 Thread Aditya Auradkar


 On Feb. 9, 2015, 5:47 p.m., Guozhang Wang wrote:
  core/src/main/scala/kafka/server/ReplicaManager.scala, lines 296-303
  https://reviews.apache.org/r/30570/diff/1/?file=846132#file846132line296
 
  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

2015-02-09 Thread Aditya Auradkar


 On Feb. 9, 2015, 5:47 p.m., Guozhang Wang wrote:
  core/src/main/scala/kafka/server/ReplicaManager.scala, lines 296-303
  https://reviews.apache.org/r/30570/diff/1/?file=846132#file846132line296
 
  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

2015-02-09 Thread Aditya Auradkar


 On Feb. 9, 2015, 6:29 p.m., Joel Koshy wrote:
  core/src/main/scala/kafka/server/ReplicaManager.scala, lines 296-303
  https://reviews.apache.org/r/30570/diff/1/?file=846132#file846132line296
 
  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

2015-02-09 Thread 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
https://reviews.apache.org/r/30570/#comment117520

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

2015-02-03 Thread 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