----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28096/#review93559 -----------------------------------------------------------
core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 45) <https://reviews.apache.org/r/28096/#comment147939> Do we really need this var or can this be passed as a parameter to the relevant methods? Avoiding mutable state is good if possible. core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 93) <https://reviews.apache.org/r/28096/#comment147937> In adddition to Gwen's comment, `breakable` is a last resort construct in Scala. So, if you can avoid using it, it's better. core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 112) <https://reviews.apache.org/r/28096/#comment147938> Simpler (and avoid discouraged `return` in Scala): `numIterations <= 0 || currentIteration < numIterations` core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 239) <https://reviews.apache.org/r/28096/#comment147933> Something along the following seems more readable and concise: `println(Seq("GROUP", "TOPIC", ...).mkString(", "))` core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 247) <https://reviews.apache.org/r/28096/#comment147934> It's a bit suspicious that `logEndOffset` and `lag` are of type `Any`. Is this really needed? core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 253) <https://reviews.apache.org/r/28096/#comment147936> Similar comment as above, better to use `Seq(...).mkString(",")` core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 330) <https://reviews.apache.org/r/28096/#comment147962> String interpolation is nicer we can use it now that we have dropped support for Scala 2.9: `s"Loop value must be greater than 0: ${opts.options.valueOf(opts.loopIntervalOpt)}"` core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 339) <https://reviews.apache.org/r/28096/#comment147961> Typo: should be `numIterations`. core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 389) <https://reviews.apache.org/r/28096/#comment147940> Space missing before `=` for a couple of `vals`. - Ismael Juma On June 24, 2015, 6:14 p.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28096/ > ----------------------------------------------------------- > > (Updated June 24, 2015, 6:14 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 ConsumerGroupCommand > > > Diffs > ----- > > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala > f23120ede5f9bf0cfaf795c65c9845f42d8784d0 > > Diff: https://reviews.apache.org/r/28096/diff/ > > > Testing > ------- > > Ran ConsumerOffsetChecker with different combinations of --output.format and > --loop options. > > > Thanks, > > Ashish Singh > >
