You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Gwen Shapira <gs...@cloudera.com> on 2015/07/29 20:35:49 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/#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 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
>
>