Re: Review Request 34789: Patch for KAFKA-2168

> On June 30, 2015, 10:24 p.m., Guozhang Wang wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > lines 933-937 > > > > > > Wondering why we want to fetch for all assigned parti

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review89971 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsum

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 30, 2015, 5:55 p.m.) Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

> On June 23, 2015, 4:20 a.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java, > > line 355 > > > > > > What's the logic to initiate connection to coordinator if

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 23, 2015, 4:39 p.m.) Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review88914 --- Ship it! Thanks for the latest patch. Looks good overall. To avoid

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 22, 2015, 11:35 p.m.) Review request for kafka. Bugs: KAFKA-216

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review88796 --- Ship it! I did not review it thoroughly but the design looks clean

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 19, 2015, 4:19 p.m.) Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

> On June 18, 2015, 11:13 p.m., Ewen Cheslack-Postava wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 1379 > > > > > > This doesn't handle nested calls properly, e.g

Re: Review Request 34789: Patch for KAFKA-2168

> On June 18, 2015, 11:13 p.m., Ewen Cheslack-Postava wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 1379 > > > > > > This doesn't handle nested calls properly, e.g

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review88451 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsum

Re: Review Request 34789: Patch for KAFKA-2168

> On June 18, 2015, 9:59 p.m., Jay Kreps wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 1364 > > > > > > This seems like one of these things that is clever but inva

Re: Review Request 34789: Patch for KAFKA-2168

> On June 18, 2015, 9:59 p.m., Jay Kreps wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 1364 > > > > > > This seems like one of these things that is clever but inva

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review88441 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsum

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 18, 2015, 9:40 p.m.) Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

> On June 15, 2015, 6:09 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 1199 > > > > > > Doesn't this cause poll() to block for the backoff time? Genera

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review87880 --- Thanks for the new patch. A few more quick comments. clients/src/m

Re: Review Request 34789: Patch for KAFKA-2168

> On June 3, 2015, 6:44 a.m., Ewen Cheslack-Postava wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/BrokerResponse.java, > > line 15 > > > > > > The classes named XResponse may be a bit con

Re: Review Request 34789: Patch for KAFKA-2168

> On June 9, 2015, 7:58 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > lines 797-798 > > > > > > Hmm, seekToBegining() is supposed to be a blocking call. Basic

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 11, 2015, 9:10 p.m.) Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

> On June 9, 2015, 7:58 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 1212 > > > > > > -1 makes the pollClient block forever. So, we don't get a chance

Re: Review Request 34789: Patch for KAFKA-2168

> On June 9, 2015, 6:49 p.m., Jay Kreps wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 322 > > > > > > Do we need this? There is no real guarantee on the poll time, so

Re: Review Request 34789: Patch for KAFKA-2168

> On June 9, 2015, 7:58 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 1212 > > > > > > -1 makes the pollClient block forever. So, we don't get a chance

Re: Review Request 34789: Patch for KAFKA-2168

> On June 9, 2015, 7:58 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 1212 > > > > > > -1 makes the pollClient block forever. So, we don't get a chance

Re: Review Request 34789: Patch for KAFKA-2168

> On June 9, 2015, 7:58 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 1212 > > > > > > -1 makes the pollClient block forever. So, we don't get a chance

Re: Review Request 34789: Patch for KAFKA-2168

> On June 9, 2015, 6:49 p.m., Jay Kreps wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 322 > > > > > > Do we need this? There is no real guarantee on the poll time, so

Re: Review Request 34789: Patch for KAFKA-2168

> On June 9, 2015, 7:58 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/OffsetResetStrategy.java, > > line 16 > > > > > > Do we need NONE? > > Jason Gustafson wrote: > It was there

Re: Review Request 34789: Patch for KAFKA-2168

> On June 9, 2015, 7:58 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 995 > > > > > > Should we pass in tp to isOffsetResetNeeded()? Yes, we should. I'l

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review87190 --- Thanks for the patch. A few comments below. clients/src/main/java/

Re: Review Request 34789: Patch for KAFKA-2168

> On June 9, 2015, 6:49 p.m., Jay Kreps wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 322 > > > > > > Do we need this? There is no real guarantee on the poll time, so

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review87257 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsum

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 5, 2015, 7:45 p.m.) Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 5, 2015, 7:02 p.m.) Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

> On June 4, 2015, 6:21 p.m., Ewen Cheslack-Postava wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 671 > > > > > > It looks like these were already issues before this c

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 4, 2015, 9:36 p.m.) Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

> On June 3, 2015, 6:44 a.m., Ewen Cheslack-Postava wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 1091 > > > > > > Since poll() can trigger auto offset commits, and t

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review86651 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsum

Re: Review Request 34789: Patch for KAFKA-2168

> On June 3, 2015, 6:44 a.m., Ewen Cheslack-Postava wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 1091 > > > > > > Since poll() can trigger auto offset commits, and t

Re: Review Request 34789: Patch for KAFKA-2168

> On June 3, 2015, 6:44 a.m., Ewen Cheslack-Postava wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 1091 > > > > > > Since poll() can trigger auto offset commits, and t

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 4, 2015, 4:07 a.m.) Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 4, 2015, 1:21 a.m.) Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

> On June 3, 2015, 6:44 a.m., Ewen Cheslack-Postava wrote: > > This looks good so far. I think it's much easier to understand when all the > > blocking stuff happens at the KafkaConsumer level and each of the classes > > it uses only ever handles single requests. It'd be nice to document the >

Re: Review Request 34789: Patch for KAFKA-2168

> On June 3, 2015, 6:44 a.m., Ewen Cheslack-Postava wrote: > > This looks good so far. I think it's much easier to understand when all the > > blocking stuff happens at the KafkaConsumer level and each of the classes > > it uses only ever handles single requests. It'd be nice to document the >

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review86338 --- This looks good so far. I think it's much easier to understand when

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 3, 2015, 12:10 a.m.) Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- (Updated June 1, 2015, 11:04 p.m.) Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

> On May 28, 2015, 11:30 p.m., Ewen Cheslack-Postava wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 857 > > > > > > I think this requires a bit more coordination betwee

Re: Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review85644 --- I think close() isn't quite right, and is probably harder than just

Review Request 34789: Patch for KAFKA-2168

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- Review request for kafka. Bugs: KAFKA-2168 https://issues.apache.org/jira/b