> On July 30, 2015, 2:11 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala, line 45 > > <https://reviews.apache.org/r/28096/diff/4/?file=991387#file991387line45> > > > > 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.
It is initialized here with a default value, however gets set just once. Moreover, it represents outputFormat for the whole class, I think it is better to have its scope for the whole class. Let me know if this still does not make sense. > On July 30, 2015, 2:11 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala, line 93 > > <https://reviews.apache.org/r/28096/diff/4/?file=991387#file991387line93> > > > > In adddition to Gwen's comment, `breakable` is a last resort construct > > in Scala. So, if you can avoid using it, it's better. Removed. Was left over from previous tries. > On July 30, 2015, 2:11 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala, line 112 > > <https://reviews.apache.org/r/28096/diff/4/?file=991387#file991387line112> > > > > Simpler (and avoid discouraged `return` in Scala): > > > > `numIterations <= 0 || currentIteration < numIterations` The code has changed a bit, so not relevant anymore. > On July 30, 2015, 2:11 p.m., Ismael Juma wrote: > > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala, line 240 > > <https://reviews.apache.org/r/28096/diff/4/?file=991387#file991387line240> > > > > Something along the following seems more readable and concise: > > > > `println(Seq("GROUP", "TOPIC", ...).mkString(", "))` Good suggestion, thanks! - Ashish ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28096/#review93559 ----------------------------------------------------------- On Aug. 5, 2015, 10:37 p.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28096/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2015, 10:37 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 > ----- > > config/server.properties 80ee2fc6e94a114e7710ae4df3f4e2b83e06f080 > 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 > >
