> On Nov. 20, 2014, 4:44 p.m., Gwen Shapira wrote: > > I'm not super-happy about adding "case" statements everywhere. I'm > > wondering if it makes sense to have a private class implementing generic > > "printPartitionInfo" and "printBrokerInfo" and encapsulate the different > > implementations. Makes sense, or is it an overkill for a small tool?
Gwen, thanks for your review! I do not think "case" can be avoided, all we can do is move it somewhere. Moving to a different class seems to be an overkill at this time. However, I have extracted printing partition info into a separate method. Let me know if this still does not look good to you. > On Nov. 20, 2014, 4:44 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/tools/ConsumerOffsetChecker.scala, lines 105-110 > > <https://reviews.apache.org/r/28096/diff/1/?file=765065#file765065line105> > > > > I really don't like creating JSON using text format. There's a Scala > > JSON library (check what the rest of Kafka is using to avoid new > > dependencies), please use that to generate valid JSON. I'd like to see one > > JSON object (Array or Map) for all consumers in the group. Thanks for pointing this out. Fixed it. > On Nov. 20, 2014, 4:44 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/tools/ConsumerOffsetChecker.scala, line 140 > > <https://reviews.apache.org/r/28096/diff/1/?file=765065#file765065line140> > > > > We are printing column names for the consumers, shouldn't we have > > column names here too? I did not get it. We are printing same info in all the cases. I might be missing something. > On Nov. 20, 2014, 4:44 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/tools/ConsumerOffsetChecker.scala, lines 212-223 > > <https://reviews.apache.org/r/28096/diff/1/?file=765065#file765065line212> > > > > Can be much cleaner using Scala's enumeration "withName". Good suggestion Gwen! Changed it to use withName. - Ashish ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28096/#review62356 ----------------------------------------------------------- On Nov. 24, 2014, 3:53 a.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28096/ > ----------------------------------------------------------- > > (Updated Nov. 24, 2014, 3:53 a.m.) > > > Review request for kafka, Gwen Shapira, Jarek Cecho, and Joel Koshy. > > > Bugs: KAFKA-313 > https://issues.apache.org/jira/browse/KAFKA-313 > > > Repository: kafka > > > Description > ------- > > KAFKA-313: Add JSON/CSV output and looping options to ConsumerOffsetChecker > > > Diffs > ----- > > core/src/main/scala/kafka/tools/ConsumerOffsetChecker.scala > d1e7c434e77859d746b8dc68dd5d5a3740425e79 > > Diff: https://reviews.apache.org/r/28096/diff/ > > > Testing > ------- > > Ran ConsumerOffsetChecker with different combinations of --output.format and > --loop options. > > > Thanks, > > Ashish Singh > >
