Re: Review Request 33242: Patch for KAFKA-2121

2015-04-20 Thread Steven Wu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 21, 2015, 5:48 a.m.) Review request for kafka. Bugs:

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-20 Thread Steven Wu
On April 21, 2015, 3:08 a.m., Guozhang Wang wrote: LGTM, besides one minor suggestion: could you move MockMetricsReporter to clients/src/test/java/org/apache/kafka/test? done. moved MockMetricsReporter to clients/src/test/java/org/apache/kafka/test - Steven

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-20 Thread Guozhang Wang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80892 --- LGTM, besides one minor suggestion: could you move

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-20 Thread Ewen Cheslack-Postava
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80770 --- Ship it! New version with KafkaConsumer changes looks good, passes

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-20 Thread Steven Wu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 4:51 p.m.) Review request for kafka. Bugs:

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-20 Thread Steven Wu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 4:57 p.m.) Review request for kafka. Bugs:

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-20 Thread Steven Wu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 4:52 p.m.) Review request for kafka. Bugs:

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-19 Thread Ewen Cheslack-Postava
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80642 --- Ship it! LGTM! If this gets merged as is, we should file a

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-19 Thread Steven Wu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 3:08 a.m.) Review request for kafka. Bugs:

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-19 Thread Steven Wu
On April 19, 2015, 11:11 p.m., Ewen Cheslack-Postava wrote: LGTM! If this gets merged as is, we should file a follow-up issue for the new consumer, which has the same issue. OK. I applied the same fix for new consumer. also updated jira title to reflect the expanded scope. - Steven

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-19 Thread Steven Wu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 3:30 a.m.) Review request for kafka. Bugs:

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-18 Thread Steven Wu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 19, 2015, 3:09 a.m.) Review request for kafka. Bugs:

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-18 Thread Steven Wu
On April 16, 2015, 5:29 p.m., Ewen Cheslack-Postava wrote: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 548 https://reviews.apache.org/r/33242/diff/2/?file=931792#file931792line548 One idea for making this less verbose and redundant: make all

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-16 Thread Steven Wu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 16, 2015, 4:55 p.m.) Review request for kafka. Bugs:

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-16 Thread Steven Wu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 16, 2015, 5:03 p.m.) Review request for kafka. Changes

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-16 Thread Steven Wu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 16, 2015, 5:44 p.m.) Review request for kafka. Bugs:

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-16 Thread Ewen Cheslack-Postava
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80346 --- Looks good, left a few comments. KafkaConsumer suffers from this

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-16 Thread Steven Wu
On April 16, 2015, 5:29 p.m., Ewen Cheslack-Postava wrote: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 531 https://reviews.apache.org/r/33242/diff/2/?file=931792#file931792line531 This code is all single threaded, is the AtomicReference really

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-16 Thread Steven Wu
On April 16, 2015, 5:29 p.m., Ewen Cheslack-Postava wrote: Looks good, left a few comments. KafkaConsumer suffers from this same problem. Patching that should be pretty much identical -- any chance you could extend this to cover that as well? sure. I can extend this to

Re: Review Request 33242: Patch for KAFKA-2121

2015-04-16 Thread Ewen Cheslack-Postava
On April 16, 2015, 5:29 p.m., Ewen Cheslack-Postava wrote: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 548 https://reviews.apache.org/r/33242/diff/2/?file=931792#file931792line548 One idea for making this less verbose and redundant: make all

Review Request 33242: Patch for KAFKA-2121

2015-04-15 Thread Steven Wu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- Review request for kafka. Bugs: KAFKA-2121