Re: Review Request 35867: Patch for KAFKA-1901

2015-08-20 Thread Manikumar Reddy O


 On Aug. 20, 2015, 2:03 a.m., Joel Koshy wrote:
  build.gradle, line 389
  https://reviews.apache.org/r/35867/diff/5/?file=1035356#file1035356line389
 
  This gives an error in detached mode (i.e., not on any branch).

Updated code to handle detached mode. thanks for the review.


- Manikumar Reddy


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


On Aug. 20, 2015, 7:08 a.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated Aug. 20, 2015, 7:08 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 Addresing Joel's comments, rebase
 
 
 Diffs
 -
 
   build.gradle 17fc223907f6b55f2d730be81ddb8d8e07a2c7ad 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 3749880b765f74af117d6c44705daf170095a1b7 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 c4621e22c32c1a1fb23726d7f56004845def96ef 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 0e7ba3ede78dbc995c404e0387a6be687703836a 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 1c1b75b4137a8b233b61739018e9cebcc3a34343 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-08-20 Thread Manikumar Reddy O

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

(Updated Aug. 20, 2015, 7:08 a.m.)


Review request for kafka.


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


Repository: kafka


Description (updated)
---

Addresing Joel's comments, rebase


Diffs (updated)
-

  build.gradle 17fc223907f6b55f2d730be81ddb8d8e07a2c7ad 
  clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
3749880b765f74af117d6c44705daf170095a1b7 
  clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
c4621e22c32c1a1fb23726d7f56004845def96ef 
  clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
PRE-CREATION 
  core/src/main/scala/kafka/common/AppInfo.scala 
d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
  core/src/main/scala/kafka/server/KafkaServer.scala 
0e7ba3ede78dbc995c404e0387a6be687703836a 
  core/src/main/scala/kafka/server/KafkaServerStartable.scala 
1c1b75b4137a8b233b61739018e9cebcc3a34343 

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


Testing
---


Thanks,

Manikumar Reddy O



Re: Review Request 35867: Patch for KAFKA-1901

2015-08-20 Thread Joel Koshy

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

Ship it!


Ship It!

- Joel Koshy


On Aug. 20, 2015, 7:08 a.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated Aug. 20, 2015, 7:08 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 Addresing Joel's comments, rebase
 
 
 Diffs
 -
 
   build.gradle 17fc223907f6b55f2d730be81ddb8d8e07a2c7ad 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 3749880b765f74af117d6c44705daf170095a1b7 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 c4621e22c32c1a1fb23726d7f56004845def96ef 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 0e7ba3ede78dbc995c404e0387a6be687703836a 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 1c1b75b4137a8b233b61739018e9cebcc3a34343 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-08-19 Thread Joel Koshy


 On July 21, 2015, 1:58 p.m., Ismael Juma wrote:
  clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java, 
  line 27
  https://reviews.apache.org/r/35867/diff/4/?file=1011317#file1011317line27
 
  Why isn't this unknown like `version`?
 
 Manikumar Reddy O wrote:
 yes we can set unknown here.  Joel suggested to use  as the 
 unknown commit ID. Joel, can we change this?

Sure - I originally thought we could make the fingerprint == the numeric value 
associated with the leading bytes. That would have made it easy to just take 
the fingerprint - convert to hex and get the git hash prefix. However, I think 
we can just drop the fingerprint altogether for now.


- Joel


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


On Aug. 9, 2015, 9:37 a.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated Aug. 9, 2015, 9:37 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 Addresing Joel's comments
 
 
 Diffs
 -
 
   build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 ed99e9bdf7c4ea7a6d4555d4488cf8ed0b80641b 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 84d4730ac634f9a5bf12a656e422fea03ad72da8 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 1c1b75b4137a8b233b61739018e9cebcc3a34343 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-08-19 Thread Joel Koshy

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


Can you rebase?


build.gradle (line 389)
https://reviews.apache.org/r/35867/#comment151107

This gives an error in detached mode (i.e., not on any branch).



clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java (line 75)
https://reviews.apache.org/r/35867/#comment151102

Sorry about the misdirection here, but I think we may as well drop this 
fingerprint. If we want to add it later, we can - and it may be useful to take 
the actual numeric value of the leading eight bytes since it makes it easier to 
instantly associate with a commit hash (otherwise we would need to tabulate 
commits to their hashCode for easy lookup).


- Joel Koshy


On Aug. 9, 2015, 9:37 a.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated Aug. 9, 2015, 9:37 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 Addresing Joel's comments
 
 
 Diffs
 -
 
   build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 ed99e9bdf7c4ea7a6d4555d4488cf8ed0b80641b 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 84d4730ac634f9a5bf12a656e422fea03ad72da8 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 1c1b75b4137a8b233b61739018e9cebcc3a34343 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-08-09 Thread Manikumar Reddy O

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

(Updated Aug. 9, 2015, 9:37 a.m.)


Review request for kafka.


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


Repository: kafka


Description
---

Addresing Joel's comments


Diffs (updated)
-

  build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f 
  clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
ed99e9bdf7c4ea7a6d4555d4488cf8ed0b80641b 
  clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 
  clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
PRE-CREATION 
  core/src/main/scala/kafka/common/AppInfo.scala 
d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
  core/src/main/scala/kafka/server/KafkaServer.scala 
84d4730ac634f9a5bf12a656e422fea03ad72da8 
  core/src/main/scala/kafka/server/KafkaServerStartable.scala 
1c1b75b4137a8b233b61739018e9cebcc3a34343 

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


Testing
---


Thanks,

Manikumar Reddy O



Re: Review Request 35867: Patch for KAFKA-1901

2015-08-09 Thread Manikumar Reddy O


 On July 21, 2015, 1:58 p.m., Ismael Juma wrote:
  clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java, 
  line 27
  https://reviews.apache.org/r/35867/diff/4/?file=1011317#file1011317line27
 
  Why isn't this unknown like `version`?

yes we can set unknown here.  Joel suggested to use  as the unknown 
commit ID. Joel, can we change this?


- Manikumar Reddy


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


On Aug. 9, 2015, 9:37 a.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated Aug. 9, 2015, 9:37 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 Addresing Joel's comments
 
 
 Diffs
 -
 
   build.gradle 1b67e628c2fca897177c12b6afad9a8700fffd1f 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 ed99e9bdf7c4ea7a6d4555d4488cf8ed0b80641b 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 84d4730ac634f9a5bf12a656e422fea03ad72da8 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 1c1b75b4137a8b233b61739018e9cebcc3a34343 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-07-21 Thread Ismael Juma

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



build.gradle (line 388)
https://reviews.apache.org/r/35867/#comment146572

Would it not be better to leave this value unset if we can't figure it out?



clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java (line 27)
https://reviews.apache.org/r/35867/#comment146573

Why isn't this unknown like `version`?


- Ismael Juma


On July 14, 2015, 12:32 p.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated July 14, 2015, 12:32 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 Addresing Joel's comments
 
 
 Diffs
 -
 
   build.gradle d86f1a8b25197d53f11e16c54a6854487e175649 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 b4e8f7f0dceefaf94a3495f39f5783cce5ceb25f 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 
   clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
 6b9590c418aedd2727544c5dd23c017b4b72467a 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
 07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
   clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
 544e120594de78c43581a980b1e4087b4fb98ccb 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 18917bc4464b9403b16d85d20c3fd4c24893d1d3 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 1c1b75b4137a8b233b61739018e9cebcc3a34343 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-07-17 Thread Joel Koshy

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



clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java (line 56)
https://reviews.apache.org/r/35867/#comment146010

given `prefix` string and further qualifies the associated AppInfo mbean 
with the attribute `id`.

Actually, sorry to bring this up again (we discussed it briefly in the 
first diff), but I think it would be better to register the app info explicitly 
in the consumer/producer/broker and remove this argument. It is convenient to 
have it instantiated as part of JmxReporter, but it is weird to have to pass in 
an `id` that will be used only for the app-info mbean.



clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java (line 
148)
https://reviews.apache.org/r/35867/#comment146037

For consistency with our regular metrics registered via KafkaMetrics maybe 
we should make this `app-info`



clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java (line 
247)
https://reviews.apache.org/r/35867/#comment146041

(for consistency) `getCommitId`
Also, `getCommitFingerprint` (fp is one word)

Also, how about these as alternates:
`getCommit` and `getCommitHash`



clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java (line 
254)
https://reviews.apache.org/r/35867/#comment146038

Can we also log the commit id and fingerprint?


- Joel Koshy


On July 14, 2015, 12:32 p.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated July 14, 2015, 12:32 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 Addresing Joel's comments
 
 
 Diffs
 -
 
   build.gradle d86f1a8b25197d53f11e16c54a6854487e175649 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 b4e8f7f0dceefaf94a3495f39f5783cce5ceb25f 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 
   clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
 6b9590c418aedd2727544c5dd23c017b4b72467a 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
 07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
   clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
 544e120594de78c43581a980b1e4087b4fb98ccb 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 18917bc4464b9403b16d85d20c3fd4c24893d1d3 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 1c1b75b4137a8b233b61739018e9cebcc3a34343 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-07-14 Thread Manikumar Reddy O


 On July 14, 2015, 1:59 a.m., Joel Koshy wrote:
  core/src/main/scala/kafka/common/AppInfo.scala, line 27
  https://reviews.apache.org/r/35867/diff/3/?file=1004947#file1004947line27
 
  Per the comment in the previous diff, I think this can go now right? 
  i.e., kafka server depends on clients so if you browse mbeans you will see 
  two app-infos registered (under `kafka.server` and `kafka.common`) which is 
  weird. The server will also expose app-info via the clients package since 
  it already uses kafka metrics and the associated jmx reporter.

Old AppInfo registration is removed. Now on Kafka Server, we will only see one 
app-info mbean.


 On July 14, 2015, 1:59 a.m., Joel Koshy wrote:
  clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java, 
  line 27
  https://reviews.apache.org/r/35867/diff/3/?file=1004944#file1004944line27
 
  (For consistency) should we make this 40-char wide as is a standard 
  commit id? Or we can just go with a eight-char or 16-char wide id for both 
  this and the actual commit id.

Ok..I am going with 16 char id.


 On July 14, 2015, 1:59 a.m., Joel Koshy wrote:
  clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java, 
  line 246
  https://reviews.apache.org/r/35867/diff/3/?file=1004943#file1004943line246
 
  In 
  https://issues.apache.org/jira/browse/KAFKA-1901?focusedCommentId=14294803page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14294803
   I was wondering if we could have a commit fingerprint - i.e., the long 
  value of the most-significant eight bytes of the commit modulo 10k or 
  something like that. This would make is convenient to register as a 
  measurable `KafkaMetric` that people can then use in their deployment 
  monitoring. i.e., instantly look at a graph and say whether all 
  brokers/clients are running the same version or not.

Implemented a simple finger print (commitId.hashcode()) mechanism. I felt 
commit.hashCode() should be sufficient for our requirement.


- Manikumar Reddy


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


On July 14, 2015, 12:32 p.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated July 14, 2015, 12:32 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 Addresing Joel's comments
 
 
 Diffs
 -
 
   build.gradle d86f1a8b25197d53f11e16c54a6854487e175649 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 b4e8f7f0dceefaf94a3495f39f5783cce5ceb25f 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 
   clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
 6b9590c418aedd2727544c5dd23c017b4b72467a 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
 07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
   clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
 544e120594de78c43581a980b1e4087b4fb98ccb 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 18917bc4464b9403b16d85d20c3fd4c24893d1d3 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 1c1b75b4137a8b233b61739018e9cebcc3a34343 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-07-14 Thread Manikumar Reddy O

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

(Updated July 14, 2015, 12:32 p.m.)


Review request for kafka.


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


Repository: kafka


Description (updated)
---

Addresing Joel's comments


Diffs (updated)
-

  build.gradle d86f1a8b25197d53f11e16c54a6854487e175649 
  clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
b4e8f7f0dceefaf94a3495f39f5783cce5ceb25f 
  clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 
  clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
6b9590c418aedd2727544c5dd23c017b4b72467a 
  clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
PRE-CREATION 
  clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
  clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
544e120594de78c43581a980b1e4087b4fb98ccb 
  core/src/main/scala/kafka/common/AppInfo.scala 
d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
  core/src/main/scala/kafka/server/KafkaServer.scala 
18917bc4464b9403b16d85d20c3fd4c24893d1d3 
  core/src/main/scala/kafka/server/KafkaServerStartable.scala 
1c1b75b4137a8b233b61739018e9cebcc3a34343 

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


Testing
---


Thanks,

Manikumar Reddy O



Re: Review Request 35867: Patch for KAFKA-1901

2015-07-13 Thread Joel Koshy

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



clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
(line 207)
https://reviews.apache.org/r/35867/#comment145018

`jmxPrefix, clientId`



clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java (line 
152)
https://reviews.apache.org/r/35867/#comment145010

`log.warn(error..., e)` - also, whitespace before ``



clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java (line 
164)
https://reviews.apache.org/r/35867/#comment145011

same



clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java (line 
246)
https://reviews.apache.org/r/35867/#comment145023

In 
https://issues.apache.org/jira/browse/KAFKA-1901?focusedCommentId=14294803page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14294803
 I was wondering if we could have a commit fingerprint - i.e., the long value 
of the most-significant eight bytes of the commit modulo 10k or something like 
that. This would make is convenient to register as a measurable `KafkaMetric` 
that people can then use in their deployment monitoring. i.e., instantly look 
at a graph and say whether all brokers/clients are running the same version or 
not.



clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java (line 27)
https://reviews.apache.org/r/35867/#comment145019

(For consistency) should we make this 40-char wide as is a standard commit 
id? Or we can just go with a eight-char or 16-char wide id for both this and 
the actual commit id.



clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java (line 33)
https://reviews.apache.org/r/35867/#comment145020

should probably reference the above constants (ln 26, 27) here instead of 
hardcoding again.



core/src/main/scala/kafka/common/AppInfo.scala (line 24)
https://reviews.apache.org/r/35867/#comment145021

Per the comment in the previous diff, I think this can go now right? i.e., 
kafka server depends on clients so if you browse mbeans you will see two 
app-infos registered (under `kafka.server` and `kafka.common`) which is weird. 
The server will also expose app-info via the clients package since it already 
uses kafka metrics and the associated jmx reporter.


- Joel Koshy


On July 10, 2015, 11:15 a.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated July 10, 2015, 11:15 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 patch after rebase
 
 
 Diffs
 -
 
   build.gradle d86f1a8b25197d53f11e16c54a6854487e175649 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 7aa076084c894bb8f47b9df2c086475b06f47060 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 
   clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
 6b9590c418aedd2727544c5dd23c017b4b72467a 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
 07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
   clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
 544e120594de78c43581a980b1e4087b4fb98ccb 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 18917bc4464b9403b16d85d20c3fd4c24893d1d3 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-07-13 Thread Joel Koshy


 On June 25, 2015, 7:01 p.m., Joel Koshy wrote:
  build.gradle, line 386
  https://reviews.apache.org/r/35867/diff/1/?file=991942#file991942line386
 
  I was originally interested in this because it would be a quick way to 
  determine what version someone is running/testing with. However, it is 
  obviously not foolproof since you could have local changes that are 
  staged/unstaged but not committed.
  
  This is probably good enough, but can you think about whether it is 
  possible to easily add a tainted boolean field? i.e., if there are any 
  additional source files that are untracked or staged but not committed?
 
 Manikumar Reddy O wrote:
 It should be posible with some git commands. But do we really need this?  
 most of us will be running stable release or some trunk point.

It may be of marginal use, but the obvious advantage is if someone reports some 
Kafka issue and _happens_ to be doing some local testing on a stable release 
along with some untracked changes.


- Joel


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


On July 10, 2015, 11:15 a.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated July 10, 2015, 11:15 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 patch after rebase
 
 
 Diffs
 -
 
   build.gradle d86f1a8b25197d53f11e16c54a6854487e175649 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 7aa076084c894bb8f47b9df2c086475b06f47060 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 
   clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
 6b9590c418aedd2727544c5dd23c017b4b72467a 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
 07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
   clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
 544e120594de78c43581a980b1e4087b4fb98ccb 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 18917bc4464b9403b16d85d20c3fd4c24893d1d3 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-07-10 Thread Manikumar Reddy O

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

(Updated July 10, 2015, 11:15 a.m.)


Review request for kafka.


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


Repository: kafka


Description (updated)
---

patch after rebase


Diffs (updated)
-

  build.gradle d86f1a8b25197d53f11e16c54a6854487e175649 
  clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
7aa076084c894bb8f47b9df2c086475b06f47060 
  clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 
  clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
6b9590c418aedd2727544c5dd23c017b4b72467a 
  clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
PRE-CREATION 
  clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
  clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
544e120594de78c43581a980b1e4087b4fb98ccb 
  core/src/main/scala/kafka/common/AppInfo.scala 
d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
  core/src/main/scala/kafka/server/KafkaServer.scala 
18917bc4464b9403b16d85d20c3fd4c24893d1d3 

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


Testing
---


Thanks,

Manikumar Reddy O



Re: Review Request 35867: Patch for KAFKA-1901

2015-06-26 Thread Manikumar Reddy O


 On June 25, 2015, 7:01 p.m., Joel Koshy wrote:
  build.gradle, line 57
  https://reviews.apache.org/r/35867/diff/1/?file=991942#file991942line57
 
  Why was this change made?

This is related to followup patch of KAFKA-2199. Will remove after KAFKA-2199 
checkin.


 On June 25, 2015, 7:01 p.m., Joel Koshy wrote:
  build.gradle, line 373
  https://reviews.apache.org/r/35867/diff/1/?file=991942#file991942line373
 
  Isn't it sufficient to just get the hash from the filesystem 
  (https://gist.github.com/JonasGroeger/7620911) instead of git log? Why do 
  we need both approaches?

yes, it should be sufficient. I just felt using git cmd is more cleaner than 
reading from filesystem.

BTW,  these gradle tasks are borrowed from gradle project itself.
https://github.com/gradle/gradle/blob/master/gradle/buildReceipt.gradle


 On June 25, 2015, 7:01 p.m., Joel Koshy wrote:
  core/src/main/scala/kafka/common/AppInfo.scala, line 17
  https://reviews.apache.org/r/35867/diff/1/?file=991949#file991949line17
 
  Do we need this AppInfo anymore? KafkaServer now initializes 
  JmxReporter - so if you browse your mbeans you will see two AppInfo mbeans 
  - one under kafka.common and another under kafka
  
  That said, I commented further above that AppInfo should probably be 
  directly instantiated and registered separately from JmxReporter.

Curently AppInfo is also used for old producer and consumer. We can remove the 
AppInfo usage from KafkaServer. But, I think we can retain this till we migrate 
kafka server to new kafka metrics.


 On June 25, 2015, 7:01 p.m., Joel Koshy wrote:
  clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java, 
  line 58
  https://reviews.apache.org/r/35867/diff/1/?file=991945#file991945line58
 
  Are you concerned about collisions with multiple clients in the same VM?

yes, this will help us to uniquely identify/register/de-register these Mbeans


 On June 25, 2015, 7:01 p.m., Joel Koshy wrote:
  clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java, 
  line 146
  https://reviews.apache.org/r/35867/diff/1/?file=991945#file991945line146
 
  Actually, why are we doing this in JmxReporter? Can we just instantiate 
  an AppInfo mbean directly in the producer/consumer/server?

Yes,  we can instantiate directly. I dont want to duplicate the code. 
JmxReporter is getting instantiated in producer,consumer and server and also 
metrics.close() will deregister these mbeans.

I can move these utility methods (registerAppInfo, unregisterAppInfo) to 
Utils.java and call directly from KafkaProducer,Consumer and Server. Let me 
know your preference. i will change accordingly.


 On June 25, 2015, 7:01 p.m., Joel Koshy wrote:
  build.gradle, line 386
  https://reviews.apache.org/r/35867/diff/1/?file=991942#file991942line386
 
  I was originally interested in this because it would be a quick way to 
  determine what version someone is running/testing with. However, it is 
  obviously not foolproof since you could have local changes that are 
  staged/unstaged but not committed.
  
  This is probably good enough, but can you think about whether it is 
  possible to easily add a tainted boolean field? i.e., if there are any 
  additional source files that are untracked or staged but not committed?

It should be posible with some git commands. But do we really need this?  most 
of us will be running stable release or some trunk point.


- Manikumar Reddy


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


On June 25, 2015, 10:11 a.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated June 25, 2015, 10:11 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 generated a properties file (kafka-version.properties) with version, 
 git-commitId in it. This props file is used to create AppInfo MBean.
 
 
 Diffs
 -
 
   build.gradle 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 9be8fbc648369ad9db1a7eea94bc1b9edbfdbfd7 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 
   clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
 6b9590c418aedd2727544c5dd23c017b4b72467a 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
 07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
   

Re: Review Request 35867: Patch for KAFKA-1901

2015-06-26 Thread Manikumar Reddy O

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

(Updated June 26, 2015, 7:49 a.m.)


Review request for kafka.


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


Repository: kafka


Description (updated)
---

Addresing Joel's comments


Diffs (updated)
-

  build.gradle 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 
  clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
9be8fbc648369ad9db1a7eea94bc1b9edbfdbfd7 
  clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 
  clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
6b9590c418aedd2727544c5dd23c017b4b72467a 
  clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
PRE-CREATION 
  clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
  clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
544e120594de78c43581a980b1e4087b4fb98ccb 
  core/src/main/scala/kafka/common/AppInfo.scala 
d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
  core/src/main/scala/kafka/server/KafkaServer.scala 
52dc728bb1ab4b05e94dc528da1006040e2f28c9 

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


Testing
---


Thanks,

Manikumar Reddy O



Review Request 35867: Patch for KAFKA-1901

2015-06-25 Thread Manikumar Reddy O

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

Review request for kafka.


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


Repository: kafka


Description
---

generated a properties file (kafka-version.properties) with version, 
git-commitId in it. This props file is used to create AppInfo MBean.


Diffs
-

  build.gradle 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 
  clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
9be8fbc648369ad9db1a7eea94bc1b9edbfdbfd7 
  clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 
  clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
6b9590c418aedd2727544c5dd23c017b4b72467a 
  clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
PRE-CREATION 
  clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
  clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
544e120594de78c43581a980b1e4087b4fb98ccb 
  core/src/main/scala/kafka/common/AppInfo.scala 
d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 

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


Testing
---


Thanks,

Manikumar Reddy O



Re: Review Request 35867: Patch for KAFKA-1901

2015-06-25 Thread Jason Rosenberg

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

Ship it!


Thanks for doing this, it all looks good from here

- Jason Rosenberg


On June 25, 2015, 10:11 a.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated June 25, 2015, 10:11 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 generated a properties file (kafka-version.properties) with version, 
 git-commitId in it. This props file is used to create AppInfo MBean.
 
 
 Diffs
 -
 
   build.gradle 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 9be8fbc648369ad9db1a7eea94bc1b9edbfdbfd7 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 
   clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
 6b9590c418aedd2727544c5dd23c017b4b72467a 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
 07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
   clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
 544e120594de78c43581a980b1e4087b4fb98ccb 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-06-25 Thread Jason Rosenberg

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



clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java (line 58)
https://reviews.apache.org/r/35867/#comment141935

what's the reason for this change?


- Jason Rosenberg


On June 25, 2015, 10:11 a.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated June 25, 2015, 10:11 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 generated a properties file (kafka-version.properties) with version, 
 git-commitId in it. This props file is used to create AppInfo MBean.
 
 
 Diffs
 -
 
   build.gradle 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 9be8fbc648369ad9db1a7eea94bc1b9edbfdbfd7 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 
   clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
 6b9590c418aedd2727544c5dd23c017b4b72467a 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
 07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
   clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
 544e120594de78c43581a980b1e4087b4fb98ccb 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-06-25 Thread Manikumar Reddy O


 On June 25, 2015, 3:33 p.m., Jason Rosenberg wrote:
  clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java, 
  line 58
  https://reviews.apache.org/r/35867/diff/1/?file=991945#file991945line58
 
  what's the reason for this change?

This change is for new java clients. Currently new java clients uses kafka 
metrics and support only JMX reporting. In this change, I am passing  clientId 
to JmxReporter. This will help use to create separate MBean for each client. 
And each MBean will have version and commitId attributes.

Example MBean:
kafka.producer:type=AppInfo,client-id=producer1
kafka.producer:type=AppInfo,client-id=producer2
kafka.consumer:type=AppInfo,client-id=consumer1
kafka.consumer:type=AppInfo,client-id=conusmer2


- Manikumar Reddy


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


On June 25, 2015, 10:11 a.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated June 25, 2015, 10:11 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 generated a properties file (kafka-version.properties) with version, 
 git-commitId in it. This props file is used to create AppInfo MBean.
 
 
 Diffs
 -
 
   build.gradle 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 9be8fbc648369ad9db1a7eea94bc1b9edbfdbfd7 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 
   clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
 6b9590c418aedd2727544c5dd23c017b4b72467a 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
 07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
   clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
 544e120594de78c43581a980b1e4087b4fb98ccb 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O
 




Re: Review Request 35867: Patch for KAFKA-1901

2015-06-25 Thread Joel Koshy

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



build.gradle (line 57)
https://reviews.apache.org/r/35867/#comment141976

Why was this change made?



build.gradle (line 367)
https://reviews.apache.org/r/35867/#comment141970

We can use `...` or `...` as the unknown commit ID.



build.gradle (line 369)
https://reviews.apache.org/r/35867/#comment141959

Can you make the whitespacing consistent (two-space)?



build.gradle (line 373)
https://reviews.apache.org/r/35867/#comment141969

Isn't it sufficient to just get the hash from the filesystem 
(https://gist.github.com/JonasGroeger/7620911) instead of git log? Why do we 
need both approaches?



build.gradle (line 386)
https://reviews.apache.org/r/35867/#comment141988

I was originally interested in this because it would be a quick way to 
determine what version someone is running/testing with. However, it is 
obviously not foolproof since you could have local changes that are 
staged/unstaged but not committed.

This is probably good enough, but can you think about whether it is 
possible to easily add a tainted boolean field? i.e., if there are any 
additional source files that are untracked or staged but not committed?



clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java (line 58)
https://reviews.apache.org/r/35867/#comment142003

Are you concerned about collisions with multiple clients in the same VM?



clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java (line 
146)
https://reviews.apache.org/r/35867/#comment142008

Actually, why are we doing this in JmxReporter? Can we just instantiate an 
AppInfo mbean directly in the producer/consumer/server?



clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java (line 
148)
https://reviews.apache.org/r/35867/#comment142011

Should we change the key from client-id to id? It is a bit weird to see an 
mbean in kafka-server with a key named client-id



clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java (line 
251)
https://reviews.apache.org/r/35867/#comment142010

Can you also log the version information (in a constructor)?



clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java (line 26)
https://reviews.apache.org/r/35867/#comment141995

unknown



clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java (line 27)
https://reviews.apache.org/r/35867/#comment141996

`...`



core/src/main/scala/kafka/common/AppInfo.scala (line 17)
https://reviews.apache.org/r/35867/#comment142001

Do we need this AppInfo anymore? KafkaServer now initializes JmxReporter - 
so if you browse your mbeans you will see two AppInfo mbeans - one under 
kafka.common and another under kafka

That said, I commented further above that AppInfo should probably be 
directly instantiated and registered separately from JmxReporter.



core/src/main/scala/kafka/common/AppInfo.scala (line 20)
https://reviews.apache.org/r/35867/#comment141983

Unused imports.


- Joel Koshy


On June 25, 2015, 10:11 a.m., Manikumar Reddy O wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35867/
 ---
 
 (Updated June 25, 2015, 10:11 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1901
 https://issues.apache.org/jira/browse/KAFKA-1901
 
 
 Repository: kafka
 
 
 Description
 ---
 
 generated a properties file (kafka-version.properties) with version, 
 git-commitId in it. This props file is used to create AppInfo MBean.
 
 
 Diffs
 -
 
   build.gradle 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 
   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
 9be8fbc648369ad9db1a7eea94bc1b9edbfdbfd7 
   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
 5671a3fbeea8cb9a9ffeeb41aa1b132b92c0cae8 
   clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java 
 6b9590c418aedd2727544c5dd23c017b4b72467a 
   clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java 
 PRE-CREATION 
   clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java 
 07b1b60d3a9cb1a399a2fe95b87229f64f539f3b 
   clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java 
 544e120594de78c43581a980b1e4087b4fb98ccb 
   core/src/main/scala/kafka/common/AppInfo.scala 
 d642ca555f83c41451d4fcaa5c01a1f86eff0a1c 
 
 Diff: https://reviews.apache.org/r/35867/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Manikumar Reddy O