You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Manikumar Reddy O <ma...@gmail.com> on 2015/06/25 12:11:30 UTC
Review Request 35867: Patch for KAFKA-1901
-----------------------------------------------------------
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
Posted by Jason Rosenberg <jb...@gmail.com>.
-----------------------------------------------------------
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
Posted by Manikumar Reddy O <ma...@gmail.com>.
> 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=14294803&page=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
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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=14294803&page=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
Posted by Manikumar Reddy O <ma...@gmail.com>.
> On July 17, 2015, 4:25 p.m., Joel Koshy wrote:
> > clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java, line 56
> > <https://reviews.apache.org/r/35867/diff/4/?file=1011316#file1011316line56>
> >
> > "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.
done the required changes to explicitly register the app info in the consumer/producer/broker.
- Manikumar Reddy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35867/#review92078
-----------------------------------------------------------
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
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
Posted by Manikumar Reddy O <ma...@gmail.com>.
> 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 00000000 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
Posted by Joel Koshy <jj...@gmail.com>.
> 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 00000000 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
Posted by Ismael Juma <mi...@juma.me.uk>.
-----------------------------------------------------------
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
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
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
Posted by Manikumar Reddy O <ma...@gmail.com>.
> 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
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
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
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
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
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
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
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
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
Re: Review Request 35867: Patch for KAFKA-1901
Posted by Joel Koshy <jj...@gmail.com>.
> 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
Posted by Manikumar Reddy O <ma...@gmail.com>.
> 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
> 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
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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 `00000000...` or `FFFF...` 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>
`0000...`
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
>
>
Re: Review Request 35867: Patch for KAFKA-1901
Posted by Manikumar Reddy O <ma...@gmail.com>.
> 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
Posted by Jason Rosenberg <jb...@gmail.com>.
-----------------------------------------------------------
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
>
>