----------------------------------------------------------- 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 > >
