----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28096/#review62356 -----------------------------------------------------------
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? core/src/main/scala/kafka/tools/ConsumerOffsetChecker.scala <https://reviews.apache.org/r/28096/#comment104397> 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. core/src/main/scala/kafka/tools/ConsumerOffsetChecker.scala <https://reviews.apache.org/r/28096/#comment104398> We are printing column names for the consumers, shouldn't we have column names here too? core/src/main/scala/kafka/tools/ConsumerOffsetChecker.scala <https://reviews.apache.org/r/28096/#comment104399> Can be much cleaner using Scala's enumeration "withName". - Gwen Shapira On Nov. 15, 2014, 6:06 p.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28096/ > ----------------------------------------------------------- > > (Updated Nov. 15, 2014, 6:06 p.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 > >
