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/07/22 08:32:50 UTC
Review Request 36681: Patch for KAFKA-2275
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-2275
https://issues.apache.org/jira/browse/KAFKA-2275
Repository: kafka
Description
-------
KAFKA-2275: Add a ListTopics() API to the new consumer
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java 9517d9d0cd480d5ba1d12f1fde7963e60528d2f8
core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
Diff: https://reviews.apache.org/r/36681/diff/
Testing
-------
Thanks,
Ashish Singh
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Ashish Singh <as...@cloudera.com>.
> On July 22, 2015, 4:43 p.m., Jason Gustafson wrote:
> > Hey Ashish, this looks pretty good to me. Just some minor comments.
Thanks for the review! Addressed your concerns.
> On July 22, 2015, 4:43 p.m., Jason Gustafson wrote:
> > clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java, line 298
> > <https://reviews.apache.org/r/36681/diff/1/?file=1018418#file1018418line298>
> >
> > Any reason not to put this method in Fetcher instead of here? I don't have a strong feeling, but it was kind of nice keeping ConsumerNetworkClient largely free of application logic.
> >
> > Also, it might be nice to have a unit test.
Moved and added unit test for Fetcher.getAllTopics.
> On July 22, 2015, 4:43 p.m., Jason Gustafson wrote:
> > clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java, lines 314-315
> > <https://reviews.apache.org/r/36681/diff/1/?file=1018418#file1018418line314>
> >
> > I think I asked this before, but is there any harm in returning this topic to the user? I ask because we don't actually prevent them from calling partitionsFor with the same topic.
Removed the exclusion.
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/#review92604
-----------------------------------------------------------
On July 22, 2015, 11:09 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36681/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 11:09 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2275
> https://issues.apache.org/jira/browse/KAFKA-2275
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Move getAllTopics from ConsumerNetworkClient to Fetcher. Add an unit test for Fetcher.getAllTopics. Do not ignore__consumer_offsets topic while getting all topics. Remove braces for single line if
>
>
> KAFKA-2275: Add a ListTopics() API to the new consumer
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
> clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
> clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java d595c1cb07909e21946532501f648be3033603d9
> clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java 7a4e586c1a0c52931a8ec55b6e1d5b67c67c28ea
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
>
> Diff: https://reviews.apache.org/r/36681/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Jason Gustafson <ja...@confluent.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/#review92604
-----------------------------------------------------------
Hey Ashish, this looks pretty good to me. Just some minor comments.
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java (line 298)
<https://reviews.apache.org/r/36681/#comment146810>
Any reason not to put this method in Fetcher instead of here? I don't have a strong feeling, but it was kind of nice keeping ConsumerNetworkClient largely free of application logic.
Also, it might be nice to have a unit test.
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java (lines 314 - 315)
<https://reviews.apache.org/r/36681/#comment146811>
I think I asked this before, but is there any harm in returning this topic to the user? I ask because we don't actually prevent them from calling partitionsFor with the same topic.
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java (line 320)
<https://reviews.apache.org/r/36681/#comment146812>
I think convention is to leave off the braces on one-line if statements.
- Jason Gustafson
On July 22, 2015, 6:32 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36681/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 6:32 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2275
> https://issues.apache.org/jira/browse/KAFKA-2275
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2275: Add a ListTopics() API to the new consumer
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
> clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
> clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java 9517d9d0cd480d5ba1d12f1fde7963e60528d2f8
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
>
> Diff: https://reviews.apache.org/r/36681/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/#review92644
-----------------------------------------------------------
Ship it!
LGTM. I think we can checkin after Jason's comments get addressed.
- Guozhang Wang
On July 22, 2015, 6:32 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36681/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 6:32 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2275
> https://issues.apache.org/jira/browse/KAFKA-2275
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2275: Add a ListTopics() API to the new consumer
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
> clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
> clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java 9517d9d0cd480d5ba1d12f1fde7963e60528d2f8
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
>
> Diff: https://reviews.apache.org/r/36681/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Edward Ribeiro <ed...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/#review92696
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java (line 167)
<https://reviews.apache.org/r/36681/#comment146934>
The code looks pretty good. Congrats!
clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java (line 201)
<https://reviews.apache.org/r/36681/#comment146932>
Just wondering to myself. If ``retryBackoffMs`` is >= than ``timeout``, then the caller will have one shot only where it can try to retrieve the topics. Not that this is a problem, as ``retryBackoffMs`` is a small amount of milliseconds, as far as I saw, but nevertheless something to be aware of.
core/src/test/scala/integration/kafka/api/ConsumerTest.scala (line 198)
<https://reviews.apache.org/r/36681/#comment146930>
nit: really *really* really cosmetic stuff, but ``topicPartsMap`` makes more sense, no? :)
- Edward Ribeiro
On July 22, 2015, 11:09 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36681/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 11:09 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2275
> https://issues.apache.org/jira/browse/KAFKA-2275
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Move getAllTopics from ConsumerNetworkClient to Fetcher. Add an unit test for Fetcher.getAllTopics. Do not ignore__consumer_offsets topic while getting all topics. Remove braces for single line if
>
>
> KAFKA-2275: Add a ListTopics() API to the new consumer
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
> clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
> clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java d595c1cb07909e21946532501f648be3033603d9
> clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java 7a4e586c1a0c52931a8ec55b6e1d5b67c67c28ea
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
>
> Diff: https://reviews.apache.org/r/36681/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Ashish Singh <as...@cloudera.com>.
> On July 28, 2015, 10:19 p.m., Guozhang Wang wrote:
> > clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java, line 42
> > <https://reviews.apache.org/r/36681/diff/3/?file=1019820#file1019820line42>
> >
> > HashMap is no longer used anywhere, and running checkStyle returns the following error:
> >
> > <file name="/Users/guozhang/Workspace/github/guozhangwang/kafka-review/clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java">
> > <error line="43" column="8" severity="error" message="Unused import - java.util.HashMap." source="com.puppycrawl.tools.checkstyle.checks.imports.UnusedImportsCheck"/>
> > </file>
> >
> > Will remove this import upon commit.
Thanks Guozhang for taking care of it!
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/#review93361
-----------------------------------------------------------
On July 23, 2015, 4:34 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36681/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 4:34 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2275
> https://issues.apache.org/jira/browse/KAFKA-2275
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Modify FetcherTest.testGetAllTopics to not use bg thread for creating request. Rename mapTopicParts -> topics
>
>
> Move getAllTopics from ConsumerNetworkClient to Fetcher. Add an unit test for Fetcher.getAllTopics. Do not ignore__consumer_offsets topic while getting all topics. Remove braces for single line if
>
>
> KAFKA-2275: Add a ListTopics() API to the new consumer
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
> clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
> clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java d595c1cb07909e21946532501f648be3033603d9
> clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java 7a4e586c1a0c52931a8ec55b6e1d5b67c67c28ea
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
>
> Diff: https://reviews.apache.org/r/36681/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/#review93361
-----------------------------------------------------------
Ship it!
clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java (line 42)
<https://reviews.apache.org/r/36681/#comment147733>
HashMap is no longer used anywhere, and running checkStyle returns the following error:
<file name="/Users/guozhang/Workspace/github/guozhangwang/kafka-review/clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java">
<error line="43" column="8" severity="error" message="Unused import - java.util.HashMap." source="com.puppycrawl.tools.checkstyle.checks.imports.UnusedImportsCheck"/>
</file>
Will remove this import upon commit.
- Guozhang Wang
On July 23, 2015, 4:34 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36681/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 4:34 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2275
> https://issues.apache.org/jira/browse/KAFKA-2275
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Modify FetcherTest.testGetAllTopics to not use bg thread for creating request. Rename mapTopicParts -> topics
>
>
> Move getAllTopics from ConsumerNetworkClient to Fetcher. Add an unit test for Fetcher.getAllTopics. Do not ignore__consumer_offsets topic while getting all topics. Remove braces for single line if
>
>
> KAFKA-2275: Add a ListTopics() API to the new consumer
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
> clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
> clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java d595c1cb07909e21946532501f648be3033603d9
> clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java 7a4e586c1a0c52931a8ec55b6e1d5b67c67c28ea
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
>
> Diff: https://reviews.apache.org/r/36681/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/
-----------------------------------------------------------
(Updated July 23, 2015, 4:34 p.m.)
Review request for kafka.
Bugs: KAFKA-2275
https://issues.apache.org/jira/browse/KAFKA-2275
Repository: kafka
Description (updated)
-------
Modify FetcherTest.testGetAllTopics to not use bg thread for creating request. Rename mapTopicParts -> topics
Move getAllTopics from ConsumerNetworkClient to Fetcher. Add an unit test for Fetcher.getAllTopics. Do not ignore__consumer_offsets topic while getting all topics. Remove braces for single line if
KAFKA-2275: Add a ListTopics() API to the new consumer
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java d595c1cb07909e21946532501f648be3033603d9
clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java 7a4e586c1a0c52931a8ec55b6e1d5b67c67c28ea
core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
Diff: https://reviews.apache.org/r/36681/diff/
Testing
-------
Thanks,
Ashish Singh
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Jason Gustafson <ja...@confluent.io>.
> On July 23, 2015, 7:19 a.m., Ashish Singh wrote:
> > clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java, line 173
> > <https://reviews.apache.org/r/36681/diff/2/?file=1019197#file1019197line173>
> >
> > getAllTopics is a blocking call and we need to send a response for it to complete, hence the thread. Let me know if it still does not makes sense.
But if you use the mockClient.prepareResponse, then you don't need the thread, right? The response will be ready when the blocking call is invoked, and it can return immediately. There are some examples of testing blocking calls in CoordinatorTest.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/#review92730
-----------------------------------------------------------
On July 22, 2015, 11:09 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36681/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 11:09 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2275
> https://issues.apache.org/jira/browse/KAFKA-2275
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Move getAllTopics from ConsumerNetworkClient to Fetcher. Add an unit test for Fetcher.getAllTopics. Do not ignore__consumer_offsets topic while getting all topics. Remove braces for single line if
>
>
> KAFKA-2275: Add a ListTopics() API to the new consumer
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
> clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
> clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java d595c1cb07909e21946532501f648be3033603d9
> clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java 7a4e586c1a0c52931a8ec55b6e1d5b67c67c28ea
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
>
> Diff: https://reviews.apache.org/r/36681/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Ismael Juma <mi...@juma.me.uk>.
> On July 23, 2015, 7:19 a.m., Ashish Singh wrote:
> > core/src/test/scala/integration/kafka/api/ConsumerTest.scala, line 198
> > <https://reviews.apache.org/r/36681/diff/2/?file=1019198#file1019198line198>
> >
> > I don't think it is an issue. Hope it's OK to drop the issue. Thanks for the review though.
I have to say that I also find the naming unusual. Normally `Map` is a suffix and not a prefix. Particularly important because `map` is also a method name and the way it's written it sounds like we're mapping something (i.e. a method name versus a noun).
- Ismael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/#review92730
-----------------------------------------------------------
On July 22, 2015, 11:09 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36681/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 11:09 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2275
> https://issues.apache.org/jira/browse/KAFKA-2275
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Move getAllTopics from ConsumerNetworkClient to Fetcher. Add an unit test for Fetcher.getAllTopics. Do not ignore__consumer_offsets topic while getting all topics. Remove braces for single line if
>
>
> KAFKA-2275: Add a ListTopics() API to the new consumer
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
> clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
> clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java d595c1cb07909e21946532501f648be3033603d9
> clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java 7a4e586c1a0c52931a8ec55b6e1d5b67c67c28ea
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
>
> Diff: https://reviews.apache.org/r/36681/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Ashish Singh <as...@cloudera.com>.
> On July 23, 2015, 7:19 a.m., Ashish Singh wrote:
> > clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java, line 173
> > <https://reviews.apache.org/r/36681/diff/2/?file=1019197#file1019197line173>
> >
> > getAllTopics is a blocking call and we need to send a response for it to complete, hence the thread. Let me know if it still does not makes sense.
>
> Jason Gustafson wrote:
> But if you use the mockClient.prepareResponse, then you don't need the thread, right? The response will be ready when the blocking call is invoked, and it can return immediately. There are some examples of testing blocking calls in CoordinatorTest.
>
> Ashish Singh wrote:
> Yes, but sending response before request sounds counter-intuitive to me. However, I do agree that it will not have any effect on expected behavior of test and reduce the complexity.
Just realized that prepareResponse adds to future responses. I missed that part earlier. Thanks for pointing out Jason!
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/#review92730
-----------------------------------------------------------
On July 23, 2015, 4:34 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36681/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 4:34 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2275
> https://issues.apache.org/jira/browse/KAFKA-2275
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Modify FetcherTest.testGetAllTopics to not use bg thread for creating request. Rename mapTopicParts -> topics
>
>
> Move getAllTopics from ConsumerNetworkClient to Fetcher. Add an unit test for Fetcher.getAllTopics. Do not ignore__consumer_offsets topic while getting all topics. Remove braces for single line if
>
>
> KAFKA-2275: Add a ListTopics() API to the new consumer
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
> clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
> clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java d595c1cb07909e21946532501f648be3033603d9
> clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java 7a4e586c1a0c52931a8ec55b6e1d5b67c67c28ea
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
>
> Diff: https://reviews.apache.org/r/36681/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Ashish Singh <as...@cloudera.com>.
> On July 23, 2015, 7:19 a.m., Ashish Singh wrote:
> > clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java, line 173
> > <https://reviews.apache.org/r/36681/diff/2/?file=1019197#file1019197line173>
> >
> > getAllTopics is a blocking call and we need to send a response for it to complete, hence the thread. Let me know if it still does not makes sense.
>
> Jason Gustafson wrote:
> But if you use the mockClient.prepareResponse, then you don't need the thread, right? The response will be ready when the blocking call is invoked, and it can return immediately. There are some examples of testing blocking calls in CoordinatorTest.
Yes, but sending response before request sounds counter-intuitive to me. However, I do agree that it will not have any effect on expected behavior of test and reduce the complexity.
> On July 23, 2015, 7:19 a.m., Ashish Singh wrote:
> > core/src/test/scala/integration/kafka/api/ConsumerTest.scala, line 198
> > <https://reviews.apache.org/r/36681/diff/2/?file=1019198#file1019198line198>
> >
> > I don't think it is an issue. Hope it's OK to drop the issue. Thanks for the review though.
>
> Ismael Juma wrote:
> I have to say that I also find the naming unusual. Normally `Map` is a suffix and not a prefix. Particularly important because `map` is also a method name and the way it's written it sounds like we're mapping something (i.e. a method name versus a noun).
Renamed it to be more inline with other usages of Map of topic and its partitions.
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/#review92730
-----------------------------------------------------------
On July 23, 2015, 4:34 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36681/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 4:34 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2275
> https://issues.apache.org/jira/browse/KAFKA-2275
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Modify FetcherTest.testGetAllTopics to not use bg thread for creating request. Rename mapTopicParts -> topics
>
>
> Move getAllTopics from ConsumerNetworkClient to Fetcher. Add an unit test for Fetcher.getAllTopics. Do not ignore__consumer_offsets topic while getting all topics. Remove braces for single line if
>
>
> KAFKA-2275: Add a ListTopics() API to the new consumer
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
> clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
> clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java d595c1cb07909e21946532501f648be3033603d9
> clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java 7a4e586c1a0c52931a8ec55b6e1d5b67c67c28ea
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
>
> Diff: https://reviews.apache.org/r/36681/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/#review92730
-----------------------------------------------------------
clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java (line 173)
<https://reviews.apache.org/r/36681/#comment146973>
getAllTopics is a blocking call and we need to send a response for it to complete, hence the thread. Let me know if it still does not makes sense.
core/src/test/scala/integration/kafka/api/ConsumerTest.scala (line 198)
<https://reviews.apache.org/r/36681/#comment146974>
I don't think it is an issue. Hope it's OK to drop the issue. Thanks for the review though.
- Ashish Singh
On July 22, 2015, 11:09 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36681/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 11:09 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2275
> https://issues.apache.org/jira/browse/KAFKA-2275
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Move getAllTopics from ConsumerNetworkClient to Fetcher. Add an unit test for Fetcher.getAllTopics. Do not ignore__consumer_offsets topic while getting all topics. Remove braces for single line if
>
>
> KAFKA-2275: Add a ListTopics() API to the new consumer
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
> clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
> clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java d595c1cb07909e21946532501f648be3033603d9
> clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java 7a4e586c1a0c52931a8ec55b6e1d5b67c67c28ea
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
>
> Diff: https://reviews.apache.org/r/36681/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Jason Gustafson <ja...@confluent.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/#review92700
-----------------------------------------------------------
Ship it!
Just one question on the unit test. Otherwise, LGTM.
clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java (line 173)
<https://reviews.apache.org/r/36681/#comment146940>
Why do we need to do this in a separate thread?
- Jason Gustafson
On July 22, 2015, 11:09 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36681/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 11:09 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2275
> https://issues.apache.org/jira/browse/KAFKA-2275
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Move getAllTopics from ConsumerNetworkClient to Fetcher. Add an unit test for Fetcher.getAllTopics. Do not ignore__consumer_offsets topic while getting all topics. Remove braces for single line if
>
>
> KAFKA-2275: Add a ListTopics() API to the new consumer
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
> clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
> clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java d595c1cb07909e21946532501f648be3033603d9
> clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java 7a4e586c1a0c52931a8ec55b6e1d5b67c67c28ea
> core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
>
> Diff: https://reviews.apache.org/r/36681/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36681: Patch for KAFKA-2275
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36681/
-----------------------------------------------------------
(Updated July 22, 2015, 11:09 p.m.)
Review request for kafka.
Bugs: KAFKA-2275
https://issues.apache.org/jira/browse/KAFKA-2275
Repository: kafka
Description (updated)
-------
Move getAllTopics from ConsumerNetworkClient to Fetcher. Add an unit test for Fetcher.getAllTopics. Do not ignore__consumer_offsets topic while getting all topics. Remove braces for single line if
KAFKA-2275: Add a ListTopics() API to the new consumer
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/ClientRequest.java ed4c0d98596cc294757f35df8c8cbc8e36ff42de
clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 48fe7961e2215372d8033ece4af739ea06c6457b
clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java 252b759c0801f392e3526b0f31503b4b8fbf1c8a
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java bea3d737c51be77d5b5293cdd944d33b905422ba
clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java c14eed1e95f2e682a235159a366046f00d1d90d6
clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java d595c1cb07909e21946532501f648be3033603d9
clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java 7a4e586c1a0c52931a8ec55b6e1d5b67c67c28ea
core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
Diff: https://reviews.apache.org/r/36681/diff/
Testing
-------
Thanks,
Ashish Singh