Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92557 --- Hey guys, I realized that most of this code has changed as the design to address this itself changed a bit. The new diff is available at https://reviews.apache.org/r/36681/. My apologies for the inconvenience, I should have vetted out design details before posting a patch. - Ashish Singh On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92286 --- clients/src/main/java/org/apache/kafka/clients/Metadata.java (line 52) https://reviews.apache.org/r/36590/#comment146348 It's a best practice to cluster fields together at the beginning of the class, so we better move this to L#43. - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92293 --- clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java (line 184) https://reviews.apache.org/r/36590/#comment146356 Same here, regarding diamond operators: MapString, ListPartitionInfo map = new HashMap(); - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92298 --- clients/src/main/java/org/apache/kafka/common/Cluster.java (line 194) https://reviews.apache.org/r/36590/#comment146362 Thif for-loop is unnecessary, as we are not doing any processing on PartitionInfo inside the loop. The for-loop can be replaced by: partitionInfos.addAll(partitionInfo); - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92301 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 414) https://reviews.apache.org/r/36590/#comment146372 Totally unrelated to this issue, but worth mentioning (imho) as the changes eventually touch this file: wouldn't be safer to make ``closed`` a volatile variable too? - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92288 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1042) https://reviews.apache.org/r/36590/#comment146351 With java7 diamonds operators this line can be simplified as: MapString, ListPartitionInfo topicAndPartitionInfoMap = new HashMap(); - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92290 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1066) https://reviews.apache.org/r/36590/#comment146353 why put this method variable as final? - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92291 --- clients/src/main/java/org/apache/kafka/common/Cluster.java (line 189) https://reviews.apache.org/r/36590/#comment146354 Same here, use diamond operators: SetPartitionInfo partitionInfos = new HashSet(); - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92294 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1072) https://reviews.apache.org/r/36590/#comment146357 why did you put this method variable as final? - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36590/#review92295
---
clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java (line
188)
https://reviews.apache.org/r/36590/#comment146359
I would rewrite this snippet as:
ListPartitionInfo parts = this.partitions.get(topic);
if (parts == null) {
parts = Collections.PartitionInfoemptyList();
}
map.put(topic, parts);
But it's more a question of taste than anything else, I confess.
- Edward Ribeiro
On July 20, 2015, 5:44 p.m., Ashish Singh wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36590/
---
(Updated July 20, 2015, 5:44 p.m.)
Review request for kafka.
Bugs: KAFKA-2275
https://issues.apache.org/jira/browse/KAFKA-2275
Repository: kafka
Description
---
Add logic to get all topics when needMetadataForAllTopics is set on metadata
Return metadata for all topics if empty list is passed to partitionsFor
KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String...
topics) API to the new consumer
Diffs
-
clients/src/main/java/org/apache/kafka/clients/Metadata.java
0387f2602c93a62cd333f1b3c569ca6b66b5b779
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
clients/src/main/java/org/apache/kafka/common/Cluster.java
60594a7dce90130911a626ea80cf80d815aeb46e
core/src/test/scala/integration/kafka/api/ConsumerTest.scala
3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
Diff: https://reviews.apache.org/r/36590/diff/
Testing
---
Thanks,
Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92302 --- clients/src/main/java/org/apache/kafka/clients/consumer/Consumer.java (line 119) https://reviews.apache.org/r/36590/#comment146373 This is unrelated to the issue (imho): declaring the acessor (i.e., ``public``) is redundant with Java interfaces as every declared method signature is public by default. Not a big deal, but worth mentioning. ;-) - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92304 --- clients/src/main/java/org/apache/kafka/common/Cluster.java (line 191) https://reviews.apache.org/r/36590/#comment146378 This if-condition is unnecessary (as of *now*). See, partitionsByTopic is defined as a final Map (L#27) so it never will be ``null``. pS: we could leave this if-condition as defensive programming for future changes, but it would never be considered a best practice make a final field non final, imho. - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description (updated) --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
On July 19, 2015, 1:11 a.m., Jason Gustafson wrote: Jason, thanks for your review! I looked into ConsumerNetworkClient/ NetwrokClient, Metadata and Cluster classes. On receiving metadataUpdate, cluster instance in metadata is updated. However, when a topic is added by consumer, it is added to metadata.topics. After considering various options, I have updated the patch with what I think is the least obtrusive changes. So, we still keep metadata.topics as the list of topics we are interested in maintaining the state for, however we can choose to get metadata for all topics by setting metadata.needMetadataForAllTopics. One thing to notice is that in the current implementation there is no caching for allTopics metadata, which might be OK depending on how we are planning to use it. We can discuss further once you take a look at the latest patch. - Ashish --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92194 --- On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92289 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1046) https://reviews.apache.org/r/36590/#comment146352 Same here. Can be simplified to: ListString missingTopics = new ArrayList(); - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36590/#review92292
---
clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java (line
196)
https://reviews.apache.org/r/36590/#comment146355
It's considered a best practice in Java to rewrite this for as:
for (Map.EntryString,ListPartitionInfo e: partitions.entrySet()) {
map.put(e.getKey(), e.getValue());
}
- Edward Ribeiro
On July 20, 2015, 5:44 p.m., Ashish Singh wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36590/
---
(Updated July 20, 2015, 5:44 p.m.)
Review request for kafka.
Bugs: KAFKA-2275
https://issues.apache.org/jira/browse/KAFKA-2275
Repository: kafka
Description
---
Add logic to get all topics when needMetadataForAllTopics is set on metadata
Return metadata for all topics if empty list is passed to partitionsFor
KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String...
topics) API to the new consumer
Diffs
-
clients/src/main/java/org/apache/kafka/clients/Metadata.java
0387f2602c93a62cd333f1b3c569ca6b66b5b779
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
clients/src/main/java/org/apache/kafka/common/Cluster.java
60594a7dce90130911a626ea80cf80d815aeb46e
core/src/test/scala/integration/kafka/api/ConsumerTest.scala
3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
Diff: https://reviews.apache.org/r/36590/diff/
Testing
---
Thanks,
Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92299 --- clients/src/main/java/org/apache/kafka/common/Cluster.java (line 192) https://reviews.apache.org/r/36590/#comment146363 Also, didn't get why yet another method variable as final. Defensive programming? I mean, what is does bring to the table? - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92297 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (lines 1065 - 1069) https://reviews.apache.org/r/36590/#comment146360 It's not a big deal, but you could move this block into the above if statement. clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1071) https://reviews.apache.org/r/36590/#comment146365 I'm not sure, but I think there might be an asynchronous issue here. Since we are using the same Cluster object in Metadata, could a pending normal metadata request (for the subscribed topics) inadvertently override our request for all metadata? clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (lines 1074 - 1077) https://reviews.apache.org/r/36590/#comment146361 Is it an actual problem if we return this topic to the user? - Jason Gustafson On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
On July 19, 2015, 1:11 a.m., Jason Gustafson wrote:
Ashish Singh wrote:
Jason, thanks for your review! I looked into ConsumerNetworkClient/
NetwrokClient, Metadata and Cluster classes. On receiving metadataUpdate,
cluster instance in metadata is updated. However, when a topic is added by
consumer, it is added to metadata.topics. After considering various options,
I have updated the patch with what I think is the least obtrusive changes.
So, we still keep metadata.topics as the list of topics we are interested in
maintaining the state for, however we can choose to get metadata for all
topics by setting metadata.needMetadataForAllTopics.
One thing to notice is that in the current implementation there is no
caching for allTopics metadata, which might be OK depending on how we are
planning to use it. We can discuss further once you take a look at the latest
patch.
Jason Gustafson wrote:
Hey Ashish, that makes sense and I agree that it seems less obtrusive.
One concern I have is that we're using the same Cluster object in Metadata
for representing both the set of all metadata and for just a subset (those
topics that have been added through subscriptions). It seems like there might
be potential for conflict there. Additionally I'm not sure how we'll be able
to extend this to handle regex subscriptions. Basically we need to be able to
listen for metadata changes and update our subscriptions based on any topic
changes. We could block to get all metdata, but it's probably best if we can
do this asynchronously. Do you have any thoughts on this?
{quote}
One concern I have is that we're using the same Cluster object in Metadata for
representing both the set of all metadata and for just a subset (those topics
that have been added through subscriptions). It seems like there might be
potential for conflict there.
{quote}
Maybe I should move the flag, indicating cluster has metadata for all topics or
subset of topics, to Cluster. Makes sense?
{quote}
Additionally I'm not sure how we'll be able to extend this to handle regex
subscriptions. Basically we need to be able to listen for metadata changes
and update our subscriptions based on any topic changes. We could block to get
all metdata, but it's probably best if we can do this asynchronously. Do you
have any thoughts on this?
{quote}
I do not think there is a way to directly subscribe to metadata changes as of
now. Correct me if my understanding is wrong. One would have to periodically
poll to get metadata updates. Now, the question becomes where should this
polling be done? With the current modification, the regex subscriber will have
to manage the polling logic. We can definitely push the polling logic down to
say Network client, but then the question will be is it required? Let me know
your thoughts.
- Ashish
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36590/#review92194
---
On July 20, 2015, 5:44 p.m., Ashish Singh wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36590/
---
(Updated July 20, 2015, 5:44 p.m.)
Review request for kafka.
Bugs: KAFKA-2275
https://issues.apache.org/jira/browse/KAFKA-2275
Repository: kafka
Description
---
Add logic to get all topics when needMetadataForAllTopics is set on metadata
Return metadata for all topics if empty list is passed to partitionsFor
KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String...
topics) API to the new consumer
Diffs
-
clients/src/main/java/org/apache/kafka/clients/Metadata.java
0387f2602c93a62cd333f1b3c569ca6b66b5b779
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
clients/src/main/java/org/apache/kafka/common/Cluster.java
60594a7dce90130911a626ea80cf80d815aeb46e
core/src/test/scala/integration/kafka/api/ConsumerTest.scala
3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
Diff: https://reviews.apache.org/r/36590/diff/
Testing
---
Thanks,
Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92307 --- clients/src/main/java/org/apache/kafka/common/Cluster.java (line 188) https://reviews.apache.org/r/36590/#comment146380 This method name is sort of a misnomer: ``pruneCluster`` for what? Firstly, it doesn't specify what it is pruning (the topics? the partitionInfo? Both?). Secondly, it is not modifying the current cluster object, but returning a new instance with only topic that have one or more ``partitionInfo``. I don't know which name would be better (pruneEmptyPartitionTopics?), but we can come up with something a bit more descriptive, I guess. :) - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92316 --- clients/src/main/java/org/apache/kafka/common/Cluster.java (line 194) https://reviews.apache.org/r/36590/#comment146392 Sorry, a correction: partitionInfos.addAll(partitions); - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92309 --- clients/src/main/java/org/apache/kafka/clients/NetworkClient.java (line 478) https://reviews.apache.org/r/36590/#comment146382 ``topics`` is a SetString. Also, it's best practice to use Collections.StringemptySet() instead of Collections.EMPTY_SET. - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92313 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1056) https://reviews.apache.org/r/36590/#comment146385 There's any reason NOT to reuse parts here? I mean, ``topicAndPartitionInfoMap.put(topic, parts)`` instead of calling ``cluster.partitionsForTopic(topic)`` again? Maybe because the partitionInfo can change under our feet between the executions of lines L#1051 and L#1056??? - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
On July 19, 2015, 1:11 a.m., Jason Gustafson wrote: Ashish Singh wrote: Jason, thanks for your review! I looked into ConsumerNetworkClient/ NetwrokClient, Metadata and Cluster classes. On receiving metadataUpdate, cluster instance in metadata is updated. However, when a topic is added by consumer, it is added to metadata.topics. After considering various options, I have updated the patch with what I think is the least obtrusive changes. So, we still keep metadata.topics as the list of topics we are interested in maintaining the state for, however we can choose to get metadata for all topics by setting metadata.needMetadataForAllTopics. One thing to notice is that in the current implementation there is no caching for allTopics metadata, which might be OK depending on how we are planning to use it. We can discuss further once you take a look at the latest patch. Hey Ashish, that makes sense and I agree that it seems less obtrusive. One concern I have is that we're using the same Cluster object in Metadata for representing both the set of all metadata and for just a subset (those topics that have been added through subscriptions). It seems like there might be potential for conflict there. Additionally I'm not sure how we'll be able to extend this to handle regex subscriptions. Basically we need to be able to listen for metadata changes and update our subscriptions based on any topic changes. We could block to get all metdata, but it's probably best if we can do this asynchronously. Do you have any thoughts on this? - Jason --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92194 --- On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92315 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1081) https://reviews.apache.org/r/36590/#comment146387 Not a big deal here, but it would be nice to return a ``topicAndPartitionInfoMap`` wrapped into a ``Collections.unmodifiableMap``. Same would be nice for original ``partitionsFor`` (a ``Collections.unmodifiableList`` in that case) - Edward Ribeiro On July 20, 2015, 5:44 p.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 20, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Add logic to get all topics when needMetadataForAllTopics is set on metadata Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - clients/src/main/java/org/apache/kafka/clients/Metadata.java 0387f2602c93a62cd333f1b3c569ca6b66b5b779 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 clients/src/main/java/org/apache/kafka/common/Cluster.java 60594a7dce90130911a626ea80cf80d815aeb46e core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36590/#review9
---
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java
(line 1069)
https://reviews.apache.org/r/36590/#comment146249
That is a good point and is actually true. The reason the test I added
worked fine is because it never added any topic to metadata, so it sent a
TopicMetadataRequest with no topics, in which case metadata for all topics is
sent back by broker. However, if I add some topics to metadata by calling {{var
mapTopicParts = this.consumers(0).partitionsFor(topic1, topic2)}} before
calling {{var mapTopicParts = this.consumers(0).partitionsFor()}} the test
fails. Will add the test and address the issues. Thanks!
- Ashish Singh
On July 18, 2015, 4:39 a.m., Ashish Singh wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36590/
---
(Updated July 18, 2015, 4:39 a.m.)
Review request for kafka.
Bugs: KAFKA-2275
https://issues.apache.org/jira/browse/KAFKA-2275
Repository: kafka
Description
---
Return metadata for all topics if empty list is passed to partitionsFor
KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String...
topics) API to the new consumer
Diffs
-
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
core/src/test/scala/integration/kafka/api/ConsumerTest.scala
3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca
Diff: https://reviews.apache.org/r/36590/diff/
Testing
---
Thanks,
Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/#review92194 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1039) https://reviews.apache.org/r/36590/#comment146216 Would it make sense to move the large part of this implementation into Fetcher? We've moved a lot of similar logic out of KafkaConsumer to keep its implementation simple. This also might make unit testing easier. clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1054) https://reviews.apache.org/r/36590/#comment146218 Adding the topic to the Metadata object means that from this point forward, we will always fetch the associated metadata for whatever topics were used in partitionsFor, even if we don't actually care about them anymore. Seems a little unfortunate, though I doubt it's much of an issue since users would probably only call this method for subscribed topics. clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1061) https://reviews.apache.org/r/36590/#comment146217 Seems like we force a metadata refresh even if there are not topics whose metadata is missing. clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1069) https://reviews.apache.org/r/36590/#comment146219 It's not clear to me how this is sufficient in order to get all topics. Wouldn't this only include topics which have been added to Metadata? Perhaps a minor concern, but I feel a little wary about mutating the Metadata state that is used internally by KafkaConsumer in this approach. Feels like there ought to be a way to request the metadata we're interested in directly instead. It would involve a change to NetworkClient, but it might be worth looking into, at least to see the level of effort. - Jason Gustafson On July 18, 2015, 4:39 a.m., Ashish Singh wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 18, 2015, 4:39 a.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - 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 core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description --- KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs - 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 core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
Re: Review Request 36590: Patch for KAFKA-2275
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36590/ --- (Updated July 18, 2015, 4:39 a.m.) Review request for kafka. Bugs: KAFKA-2275 https://issues.apache.org/jira/browse/KAFKA-2275 Repository: kafka Description (updated) --- Return metadata for all topics if empty list is passed to partitionsFor KAFKA-2275: Add a MapString, ListPartitionInfo partitionsFor(String... topics) API to the new consumer Diffs (updated) - 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 core/src/test/scala/integration/kafka/api/ConsumerTest.scala 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca Diff: https://reviews.apache.org/r/36590/diff/ Testing --- Thanks, Ashish Singh
