> On July 29, 2015, 6:35 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala, lines 36-39 > > <https://reviews.apache.org/r/28096/diff/4/?file=991387#file991387line36> > > > > Shouldn't this be an inner object? since its only visible and used by > > ConsumerGroupCommand?
Makes sense :) > On July 29, 2015, 6:35 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala, line 93 > > <https://reviews.apache.org/r/28096/diff/4/?file=991387#file991387line93> > > > > This is defined as "breakable", but I don't see where you use break? There was one, which I removed and forgot to get rid of this. Thanks for catching this. > On July 29, 2015, 6:35 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala, lines 109-113 > > <https://reviews.apache.org/r/28096/diff/4/?file=991387#file991387line109> > > > > Since its a boolean, we want a name that reflects what we check. > > Perhaps "iterationsLeft"? > > > > it looks like we want to return true if we want to continue iterating - > > in this case shouldn't a negative numIterations lead to false? Changed the func name. For -ve numIterations, I was thinking that if user has put some invalid value then probably we should skip the numIterations check. However, I realize that a user intentionally might want to have a -ve value for some reason that I can not think of. So changed. > On July 29, 2015, 6:35 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala, lines 237-242 > > <https://reviews.apache.org/r/28096/diff/4/?file=991387#file991387line237> > > > > These look identical - copy/paste error? Not really. There is some difference, None has "%s, %s" format, while CSV has "%s,%s" format. > On July 29, 2015, 6:35 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala, line 250 > > <https://reviews.apache.org/r/28096/diff/4/?file=991387#file991387line250> > > > > I thought none is tab-delimited, not CSV? My idea was that I will keep None correspond to exisiting format, which is "%s, %s". > On July 29, 2015, 6:35 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala, line 389 > > <https://reviews.apache.org/r/28096/diff/4/?file=991387#file991387line389> > > > > If only CSV and JSON are allowed, what is NONE for? If outputFormat is not specified, it will maintain the existing output format, that is what None is for. Makes sense? > On July 29, 2015, 6:35 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala, line 364 > > <https://reviews.apache.org/r/28096/diff/4/?file=991387#file991387line364> > > > > Kafka code base usually doesn't use methods as operators. Why are we > > doing this here? > > > > Also, why define "ofTypes" here? Removed ofTypes. - Ashish ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28096/#review93489 ----------------------------------------------------------- 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 > >
