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