You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Ashish Singh <as...@cloudera.com> on 2015/06/24 20:14:36 UTC
Re: Review Request 28096: Patch for KAFKA-313
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 28096: Patch for KAFKA-313
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28096/
-----------------------------------------------------------
(Updated Aug. 12, 2015, 9:21 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 (updated)
-------
Remove NONE output-format
KAFKA-313: Add JSON/CSV output and looping options to ConsumerGroupCommand
Diffs (updated)
-----
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
Re: Review Request 28096: Patch for KAFKA-313
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28096/
-----------------------------------------------------------
(Updated Aug. 10, 2015, 7:58 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 (updated)
-----
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
Re: Review Request 28096: Patch for KAFKA-313
Posted by Edward Ribeiro <ed...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28096/#review94522
-----------------------------------------------------------
core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 107)
<https://reviews.apache.org/r/28096/#comment149146>
nit: ``numInterations`` don't need to be a ``var`` (the less vars the better, right?)
It's a matter of deleting L#107, making L#109 as ``val numIterations = opts.options.valueOf(opts.numIterationsOpt).toInt`` and moving L#117 to L#113 (shift former L#113 down). This has the benefit of making the ``return`` keyword unnecessary at L#114.
That is:
``
if (opts.options.has(opts.numIterationsOpt)) {
val numIterations = opts.options.valueOf(opts.numIterationsOpt).toInt
if (numIterations < 0)
CommandLineUtils.printUsageAndDie(opts.parser, s"Loop value must be greater than 0:${opts.options.valueOf(opts.loopIntervalOpt)}")
currentIteration < numIterations
} else {
true // keep iterating, if iterations not limited by numIterations
}
``
core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 335)
<https://reviews.apache.org/r/28096/#comment149149>
single line commands don't use brackets, right? L#335 and L#338.
core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 345)
<https://reviews.apache.org/r/28096/#comment149145>
nit: you could rename this as ``elapsedTime`` and make it ``System.currentTimeMillis() - startTime`` as it's only used at line L#346. So, L#346 becomes ``val sleepTime = loopInterval - elapsedTime``.
- Edward Ribeiro
On Aug. 5, 2015, 10:43 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:43 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
>
>
Re: Review Request 28096: Patch for KAFKA-313
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28096/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 10:43 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 (updated)
-----
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
Re: Review Request 28096: Patch for KAFKA-313
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 28096: Patch for KAFKA-313
Posted by Ashish Singh <as...@cloudera.com>.
> 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
>
>
Re: Review Request 28096: Patch for KAFKA-313
Posted by Ashish Singh <as...@cloudera.com>.
> 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?
>
> Ashish Singh wrote:
> Not really. There is some difference, None has "%s, %s" format, while CSV has "%s,%s" format.
>
> Gwen Shapira wrote:
> The KIP (https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=56852556) doesn't mention NONE.
> Since CSV and NONE are so similar (just a matter of an extra space), does it make sense to just drop NONE? (which was my expectation, given the KIP)
Gwen, I updated the patch to only have CSV and JSON as output-formats.
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28096/#review93489
-----------------------------------------------------------
On Aug. 12, 2015, 9:21 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28096/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2015, 9:21 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
> -------
>
> Remove NONE output-format
>
>
> 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
>
>
Re: Review Request 28096: Patch for KAFKA-313
Posted by Gwen Shapira <gs...@cloudera.com>.
> 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?
>
> Ashish Singh wrote:
> Not really. There is some difference, None has "%s, %s" format, while CSV has "%s,%s" format.
The KIP (https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=56852556) doesn't mention NONE.
Since CSV and NONE are so similar (just a matter of an extra space), does it make sense to just drop NONE? (which was my expectation, given the KIP)
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28096/#review93489
-----------------------------------------------------------
On Aug. 10, 2015, 7:58 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28096/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 7:58 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
>
>
Re: Review Request 28096: Patch for KAFKA-313
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28096/#review93489
-----------------------------------------------------------
Thanks for the patch and sorry for the long delay.
core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (lines 36 - 39)
<https://reviews.apache.org/r/28096/#comment147867>
Shouldn't this be an inner object? since its only visible and used by ConsumerGroupCommand?
core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 93)
<https://reviews.apache.org/r/28096/#comment147875>
This is defined as "breakable", but I don't see where you use break?
core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (lines 109 - 113)
<https://reviews.apache.org/r/28096/#comment147878>
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?
core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (lines 236 - 241)
<https://reviews.apache.org/r/28096/#comment147880>
These look identical - copy/paste error?
core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 249)
<https://reviews.apache.org/r/28096/#comment147881>
I thought none is tab-delimited, not CSV?
core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 363)
<https://reviews.apache.org/r/28096/#comment147871>
Kafka code base usually doesn't use methods as operators. Why are we doing this here?
Also, why define "ofTypes" here?
core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala (line 388)
<https://reviews.apache.org/r/28096/#comment147883>
If only CSV and JSON are allowed, what is NONE for?
- Gwen Shapira
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
>
>
Re: Review Request 28096: Patch for KAFKA-313
Posted by Ashish Singh <as...@cloudera.com>.
> 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
>
>
Re: Review Request 28096: Patch for KAFKA-313
Posted by Ismael Juma <mi...@juma.me.uk>.
-----------------------------------------------------------
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
>
>