----------------------------------------------------------- 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 "Map<String, List<PartitionInfo>> 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 > >
