Re: Review Request 34789: Patch for KAFKA-2168

2015-06-30 Thread Jason Gustafson
--- 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:

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-30 Thread Jason Gustafson
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 https://reviews.apache.org/r/34789/diff/14/?file=996120#file996120line933 Wondering why we want to fetch for all assigned partitions if the

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-30 Thread Guozhang Wang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review89971 ---

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-23 Thread Jason Gustafson
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 https://reviews.apache.org/r/34789/diff/12/?file=990006#file990006line355 What's the logic to initiate connection to coordinator if the

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-23 Thread Jason Gustafson
--- 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:

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-22 Thread Guozhang Wang
--- 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

2015-06-22 Thread Jason Gustafson
--- 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:

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-22 Thread Jun Rao
--- 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

2015-06-19 Thread Jason Gustafson
--- 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:

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-18 Thread Jason Gustafson
On June 18, 2015, 9:59 p.m., Jay Kreps wrote: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 1364 https://reviews.apache.org/r/34789/diff/9-10/?file=983137#file983137line1364 This seems like one of these things that is clever but invariably ends

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-18 Thread Jason Gustafson
--- 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:

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-18 Thread Jay Kreps
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review88441 ---

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-18 Thread Ewen Cheslack-Postava
On June 18, 2015, 9:59 p.m., Jay Kreps wrote: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 1364 https://reviews.apache.org/r/34789/diff/9-10/?file=983137#file983137line1364 This seems like one of these things that is clever but invariably ends

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-18 Thread Ewen Cheslack-Postava
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review88451 ---

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-18 Thread Jason Gustafson
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 https://reviews.apache.org/r/34789/diff/9-10/?file=983137#file983137line1379 This doesn't handle nested calls properly, e.g. poll() -

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-18 Thread Ewen Cheslack-Postava
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 https://reviews.apache.org/r/34789/diff/9-10/?file=983137#file983137line1379 This doesn't handle nested calls properly, e.g. poll() -

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-16 Thread Jason Gustafson
On June 15, 2015, 6:09 p.m., Jun Rao wrote: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 1199 https://reviews.apache.org/r/34789/diff/9/?file=983137#file983137line1199 Doesn't this cause poll() to block for the backoff time? Generally it

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-15 Thread Jun Rao
--- 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.

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-11 Thread Jason Gustafson
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 https://reviews.apache.org/r/34789/diff/8/?file=980075#file980075line797 Hmm, seekToBegining() is supposed to be a blocking call. Basically, at

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-11 Thread Jason Gustafson
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 https://reviews.apache.org/r/34789/diff/3/?file=976967#file976967line15 The classes named XResponse may be a bit confusing

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-11 Thread Jason Gustafson
--- 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:

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-10 Thread Ewen Cheslack-Postava
On June 9, 2015, 7:58 p.m., Jun Rao wrote: clients/src/main/java/org/apache/kafka/clients/consumer/OffsetResetStrategy.java, line 16 https://reviews.apache.org/r/34789/diff/8/?file=980077#file980077line16 Do we need NONE? Jason Gustafson wrote: It was there before, but I

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-10 Thread Jun Rao
On June 9, 2015, 6:49 p.m., Jay Kreps wrote: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 322 https://reviews.apache.org/r/34789/diff/8/?file=980075#file980075line322 Do we need this? There is no real guarantee on the poll time, so it seems

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-10 Thread Jun Rao
On June 9, 2015, 7:58 p.m., Jun Rao wrote: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 1212 https://reviews.apache.org/r/34789/diff/8/?file=980075#file980075line1212 -1 makes the pollClient block forever. So, we don't get a chance to do the

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-10 Thread Jason Gustafson
On June 9, 2015, 6:49 p.m., Jay Kreps wrote: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 322 https://reviews.apache.org/r/34789/diff/8/?file=980075#file980075line322 Do we need this? There is no real guarantee on the poll time, so it seems

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-10 Thread Jun Rao
On June 9, 2015, 7:58 p.m., Jun Rao wrote: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 1212 https://reviews.apache.org/r/34789/diff/8/?file=980075#file980075line1212 -1 makes the pollClient block forever. So, we don't get a chance to do the

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-10 Thread Jason Gustafson
On June 9, 2015, 7:58 p.m., Jun Rao wrote: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 1212 https://reviews.apache.org/r/34789/diff/8/?file=980075#file980075line1212 -1 makes the pollClient block forever. So, we don't get a chance to do the

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-10 Thread Jun Rao
On June 9, 2015, 7:58 p.m., Jun Rao wrote: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 1212 https://reviews.apache.org/r/34789/diff/8/?file=980075#file980075line1212 -1 makes the pollClient block forever. So, we don't get a chance to do the

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-09 Thread Jay Kreps
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review87257 ---

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-09 Thread Jason Gustafson
On June 9, 2015, 6:49 p.m., Jay Kreps wrote: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 322 https://reviews.apache.org/r/34789/diff/8/?file=980075#file980075line322 Do we need this? There is no real guarantee on the poll time, so it seems

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-09 Thread Jun Rao
--- 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.

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-09 Thread Jason Gustafson
On June 9, 2015, 7:58 p.m., Jun Rao wrote: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 995 https://reviews.apache.org/r/34789/diff/8/?file=980075#file980075line995 Should we pass in tp to isOffsetResetNeeded()? Yes, we should. I'll fix it.

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-05 Thread Jason Gustafson
--- 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

2015-06-05 Thread Jason Gustafson
--- 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

2015-06-04 Thread Jason Gustafson
--- 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

2015-06-04 Thread Jason Gustafson
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 https://reviews.apache.org/r/34789/diff/5/?file=978185#file978185line671 It looks like these were already issues before this code was

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-04 Thread Ewen Cheslack-Postava
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 https://reviews.apache.org/r/34789/diff/3/?file=976965#file976965line1091 Since poll() can trigger auto offset commits, and then the

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-04 Thread Jason Gustafson
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 https://reviews.apache.org/r/34789/diff/3/?file=976965#file976965line1091 Since poll() can trigger auto offset commits, and then the

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-04 Thread Ewen Cheslack-Postava
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/#review86651 ---

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-04 Thread Ewen Cheslack-Postava
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 https://reviews.apache.org/r/34789/diff/3/?file=976965#file976965line1091 Since poll() can trigger auto offset commits, and then the

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-03 Thread Ewen Cheslack-Postava
--- 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

2015-06-03 Thread Jason Gustafson
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 basic

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-03 Thread Jason Gustafson
--- 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

2015-06-03 Thread Jason Gustafson
--- 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

2015-06-03 Thread Jason Gustafson
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 basic

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-02 Thread Jason Gustafson
--- 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:

Re: Review Request 34789: Patch for KAFKA-2168

2015-06-01 Thread Jason Gustafson
--- 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:

Review Request 34789: Patch for KAFKA-2168

2015-05-28 Thread Jason Gustafson
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34789/ --- Review request for kafka. Bugs: KAFKA-2168

Re: Review Request 34789: Patch for KAFKA-2168

2015-05-28 Thread Ewen Cheslack-Postava
--- 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

Re: Review Request 34789: Patch for KAFKA-2168

2015-05-28 Thread Jason Gustafson
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 https://reviews.apache.org/r/34789/diff/1/?file=973873#file973873line857 I think this requires a bit more coordination between the