Re: Review Request 29467: Patch for KAFKA-1660
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review75656 --- I just had a patch submitted for KAFKA-1660. But I do not have permission to upload patch to this rb, so I created another one: https://reviews.apache.org/r/31850/ - Jiangjie Qin On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated March 2, 2015, 6:41 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Changing log levels as suggested. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 7397e565fd865214529ffccadd4222d835ac8110 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6913090af03a455452b0b5c3df78f266126b3854 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5b3e75ed83a940bc922f9eca10d4008d67aa37c9 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
On March 3, 2015, 4:10 a.m., Jay Kreps wrote: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 560 https://reviews.apache.org/r/29467/diff/4/?file=882247#file882247line560 This seems to call initiateClose() twice, once in initiateClose and then again from forceClose. This seems like it depends on all the things getting closed being idempotent to repeated calls (e.g. record accumulator etc). Would it make more sense to have forceClose() just set the force flag? The issue with that is someone can just call sender.forceClose and it will never call accumulator.close which is part of initiate close. Also shouldn't the calls be idempotent given this can be called from multiple threds multiple times? On March 3, 2015, 4:10 a.m., Jay Kreps wrote: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 554 https://reviews.apache.org/r/29467/diff/4/?file=882247#file882247line554 It's probably worth adding an if(timeout 0) on this. Added. On March 3, 2015, 4:10 a.m., Parth Brahmbhatt wrote: Two minor changes I noted, but otherwise looks good to me. Needs some unit tests, as you mentioned. Jay Kreps wrote: Actually one probably I didn't think of is that forceClose() leaves the in-flight requests forever incomplete. A better approach would be to fail them all with TimeoutException. To do this correctly I will need to get the imcomplete and unsent RecordBatches from RecordAccumulator. I can add methods to get these with default scope. The sender will need these to emit correct metrics and failing the bathces. For unit testing I need someway to mock RecordAccumulator as the Seneder's run method where the force close logic lives is a while(true) loop which dependes on the values of record accumulator. RecordAccumulator is a final class right now, is it ok to change that so I can create a MockRecordAccumulator? - Parth --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review74884 --- On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated March 2, 2015, 6:41 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Changing log levels as suggested. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 7397e565fd865214529ffccadd4222d835ac8110 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6913090af03a455452b0b5c3df78f266126b3854 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5b3e75ed83a940bc922f9eca10d4008d67aa37c9 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
On March 3, 2015, 5:37 a.m., Jay Kreps wrote:
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java,
line 533
https://reviews.apache.org/r/29467/diff/4/?file=882247#file882247line533
Now there is a bit of duplicate code between the two close methods.
Maybe this would be cleaner if we just made
public void close() {
close(Long.MAX_VALUE, TimeUnit.MILLISECONDS);
}
Fixed.
- Parth
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29467/#review74897
---
On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29467/
---
(Updated March 2, 2015, 6:41 p.m.)
Review request for kafka.
Bugs: KAFKA-1660
https://issues.apache.org/jira/browse/KAFKA-1660
Repository: kafka
Description
---
Merge remote-tracking branch 'origin/trunk' into KAFKA-1660
Conflicts:
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
Merge remote-tracking branch 'origin/trunk' into KAFKA-1660
Changing log levels as suggested.
Diffs
-
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
7397e565fd865214529ffccadd4222d835ac8110
clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java
6913090af03a455452b0b5c3df78f266126b3854
clients/src/main/java/org/apache/kafka/clients/producer/Producer.java
5b3e75ed83a940bc922f9eca10d4008d67aa37c9
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
ed9c63a6679e3aaf83d19fde19268553a4c107c2
Diff: https://reviews.apache.org/r/29467/diff/
Testing
---
existing unit tests passed.
Thanks,
Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review74833 --- Ship it! LGTM. - Jiangjie Qin On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated March 2, 2015, 6:41 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Changing log levels as suggested. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 7397e565fd865214529ffccadd4222d835ac8110 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6913090af03a455452b0b5c3df78f266126b3854 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5b3e75ed83a940bc922f9eca10d4008d67aa37c9 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review74835 --- Could we add some unit tests for this new API as I mentioned in my previous comment? - Guozhang Wang On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated March 2, 2015, 6:41 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Changing log levels as suggested. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 7397e565fd865214529ffccadd4222d835ac8110 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6913090af03a455452b0b5c3df78f266126b3854 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5b3e75ed83a940bc922f9eca10d4008d67aa37c9 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review74843 --- Actually I spoke too fast... As the flush() has been checked in, we need to take care of the caller thread that are doing a flush when invoking close(). This is a little bit tricky. If we close the producer forcibily when caller thread were doing a flush, we have to notify the caller thread that the flush failed. The simplest way might be letting flush return a boolean value. So we do the following: 1. In RecordAccumulator add a new forceClose(), it sets an forceClosed flag first, then clear up the imcomplete batchset and wake up all the caller threads. 2. In RecordAccumulator.awaitFlushCompletion(), it checks the forceClosed flag to determine whether flush succeeded or not and return the result to KafkaProducer.flush(). 3. KafkaProducer.flush() return this result to caller threads. clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java https://reviews.apache.org/r/29467/#comment121626 We probably need to release the caller threads that are waiting on flush() at this point. - Jiangjie Qin On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated March 2, 2015, 6:41 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Changing log levels as suggested. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 7397e565fd865214529ffccadd4222d835ac8110 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6913090af03a455452b0b5c3df78f266126b3854 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5b3e75ed83a940bc922f9eca10d4008d67aa37c9 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
On March 2, 2015, 11:04 p.m., Jiangjie Qin wrote: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java, line 219 https://reviews.apache.org/r/29467/diff/4/?file=882250#file882250line219 We probably need to release the caller threads that are waiting on flush() at this point. Making flush a boolean method that you have to always check to see if someone called close() in another thead would be a really really really painful api to use in practice, right? I think the issue here is actually what I pointed out in the other comment, namely that in-flight requests area actually left incomplete when you call close and hit the forceClose timeout. Any other thread blocking on these futures would block forever. The right solution is just to fail all requests that haven't completed when forceClose kicks in. This then fullfills the criteria for flush which is that all the requests are completed or failed. - Jay --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review74843 --- On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated March 2, 2015, 6:41 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Changing log levels as suggested. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 7397e565fd865214529ffccadd4222d835ac8110 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6913090af03a455452b0b5c3df78f266126b3854 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5b3e75ed83a940bc922f9eca10d4008d67aa37c9 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
On March 2, 2015, 11:04 p.m., Jiangjie Qin wrote: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java, line 219 https://reviews.apache.org/r/29467/diff/4/?file=882250#file882250line219 We probably need to release the caller threads that are waiting on flush() at this point. Jay Kreps wrote: Making flush a boolean method that you have to always check to see if someone called close() in another thead would be a really really really painful api to use in practice, right? I think the issue here is actually what I pointed out in the other comment, namely that in-flight requests area actually left incomplete when you call close and hit the forceClose timeout. Any other thread blocking on these futures would block forever. The right solution is just to fail all requests that haven't completed when forceClose kicks in. This then fullfills the criteria for flush which is that all the requests are completed or failed. Yes, I agree that letting flush() return a boolean to just indicate whether someone called close is ugly. I'm thinking maybe we can make the return value to be more useful. The idea of letting flush return a boolean comes when I was writing the mirror maker. When we call flush() followed by a consumer.commitOffsets(), we need to know the result of flush() in order to decide whether to commit offset or not. There might be three cases: 1. flush() succeeded on all batches. 2. flush() failed and some exception were thrown to caller thread (very rare, InterruptedException maybe) 3. flush() failed but are handled by sender thread in send callbacks. For 1), no problem, everybody is happy. For 2), caller thread knows something wrong happened and will not do next task (i.e. commit offsets). For 3), caller thread has no idea about what happened and assumes everthing went well. What I'm doing now is in send callback let the sender thread set a flag for the caller thread to check whether the flush succeeded or not when flush() returns. Otherwise, caller thread cannot decide whether to commit offset or not. I'm thinking if in most cases people care about whether flush succeeded or not, they need to have this inter thread communication. If it is a common requirement, maybe we can let flush() return a boolean. From API point of view, it is probably OK. If user cares about whether flush succeeded or not, they check the return value, otherwise they ignore it. Just like the what we do for send(). - Jiangjie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review74843 --- On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated March 2, 2015, 6:41 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Changing log levels as suggested. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 7397e565fd865214529ffccadd4222d835ac8110 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6913090af03a455452b0b5c3df78f266126b3854 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5b3e75ed83a940bc922f9eca10d4008d67aa37c9 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review74884 --- clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java https://reviews.apache.org/r/29467/#comment121709 It's probably worth adding an if(timeout 0) on this. clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java https://reviews.apache.org/r/29467/#comment121708 This seems to call initiateClose() twice, once in initiateClose and then again from forceClose. This seems like it depends on all the things getting closed being idempotent to repeated calls (e.g. record accumulator etc). Would it make more sense to have forceClose() just set the force flag? Two minor changes I noted, but otherwise looks good to me. Needs some unit tests, as you mentioned. - Jay Kreps On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated March 2, 2015, 6:41 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Changing log levels as suggested. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 7397e565fd865214529ffccadd4222d835ac8110 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6913090af03a455452b0b5c3df78f266126b3854 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5b3e75ed83a940bc922f9eca10d4008d67aa37c9 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29467/#review74897
---
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
https://reviews.apache.org/r/29467/#comment121725
Now there is a bit of duplicate code between the two close methods. Maybe
this would be cleaner if we just made
public void close() {
close(Long.MAX_VALUE, TimeUnit.MILLISECONDS);
}
- Jay Kreps
On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29467/
---
(Updated March 2, 2015, 6:41 p.m.)
Review request for kafka.
Bugs: KAFKA-1660
https://issues.apache.org/jira/browse/KAFKA-1660
Repository: kafka
Description
---
Merge remote-tracking branch 'origin/trunk' into KAFKA-1660
Conflicts:
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
Merge remote-tracking branch 'origin/trunk' into KAFKA-1660
Changing log levels as suggested.
Diffs
-
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
7397e565fd865214529ffccadd4222d835ac8110
clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java
6913090af03a455452b0b5c3df78f266126b3854
clients/src/main/java/org/apache/kafka/clients/producer/Producer.java
5b3e75ed83a940bc922f9eca10d4008d67aa37c9
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
ed9c63a6679e3aaf83d19fde19268553a4c107c2
Diff: https://reviews.apache.org/r/29467/diff/
Testing
---
existing unit tests passed.
Thanks,
Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
On March 3, 2015, 4:10 a.m., Parth Brahmbhatt wrote: Two minor changes I noted, but otherwise looks good to me. Needs some unit tests, as you mentioned. Actually one probably I didn't think of is that forceClose() leaves the in-flight requests forever incomplete. A better approach would be to fail them all with TimeoutException. - Jay --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review74884 --- On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated March 2, 2015, 6:41 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Changing log levels as suggested. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 7397e565fd865214529ffccadd4222d835ac8110 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6913090af03a455452b0b5c3df78f266126b3854 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5b3e75ed83a940bc922f9eca10d4008d67aa37c9 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review74777 --- clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java https://reviews.apache.org/r/29467/#comment121523 Changed log level as suggested. clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java https://reviews.apache.org/r/29467/#comment121524 included. clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java https://reviews.apache.org/r/29467/#comment121525 changed log level to suggested value. - Parth Brahmbhatt On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated March 2, 2015, 6:41 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Changing log levels as suggested. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 7397e565fd865214529ffccadd4222d835ac8110 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6913090af03a455452b0b5c3df78f266126b3854 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5b3e75ed83a940bc922f9eca10d4008d67aa37c9 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated March 2, 2015, 6:41 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description (updated) --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Changing log levels as suggested. Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 7397e565fd865214529ffccadd4222d835ac8110 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6913090af03a455452b0b5c3df78f266126b3854 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5b3e75ed83a940bc922f9eca10d4008d67aa37c9 clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review74149 --- Could we add some unit tests for this new API, both called by another caller thread as well as in the callback. Things to check: 1. Concurrent calls to close(). 2. Timeout value is honered in close(). 3. If producer is closed immediately no buffered messages will get sent. clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java https://reviews.apache.org/r/29467/#comment120674 I think we will use debug level logging here indicating function returning: https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Error+Handling+and+Logging clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java https://reviews.apache.org/r/29467/#comment120680 Include the timeout value in the log entry? clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java https://reviews.apache.org/r/29467/#comment120675 Ditto. - Guozhang Wang On Feb. 18, 2015, 12:41 a.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated Feb. 18, 2015, 12:41 a.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 1fd6917c8a5131254c740abad7f7228a47e3628c clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 84530f2b948f9abd74203db48707e490dd9c81a5 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 17fe541588d462c68c33f6209717cc4015e9b62f clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated Feb. 18, 2015, 12:41 a.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 1fd6917c8a5131254c740abad7f7228a47e3628c clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 84530f2b948f9abd74203db48707e490dd9c81a5 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 17fe541588d462c68c33f6209717cc4015e9b62f clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated Feb. 18, 2015, 12:36 a.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description (updated) --- Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 Conflicts: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 1fd6917c8a5131254c740abad7f7228a47e3628c clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 84530f2b948f9abd74203db48707e490dd9c81a5 clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 17fe541588d462c68c33f6209717cc4015e9b62f clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java ed9c63a6679e3aaf83d19fde19268553a4c107c2 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review71595 --- I would vote for the name close(long timeout, TimeUnit unit) I think the params make it clear that it is an attempt and we can clarify that in the docs too. clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java https://reviews.apache.org/r/29467/#comment117410 This approach will actually leak the sender thread if there are still unsent requests. I think this is not what people want. I think what they want is for the sender thread to attempt to send their messages for N ms and then shutdown if it still hasn't succeeded. Leaking the thread seems like a bug. - Jay Kreps On Dec. 29, 2014, 10:52 p.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated Dec. 29, 2014, 10:52 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- KAFKA-1660: Adding tryClose(timeoutMillis) to producer. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java f61efb35db7e0de590556e6a94a7b5cb850cdae9 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 34624c3b7a1f28735ab6c63cc9e18a410e87e63c clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5baa6062bd9ba8a7d38058856ed2d831fae491f0 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
On Feb. 9, 2015, 1:27 a.m., Jay Kreps wrote: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 371 https://reviews.apache.org/r/29467/diff/1/?file=802888#file802888line371 This approach will actually leak the sender thread if there are still unsent requests. I think this is not what people want. I think what they want is for the sender thread to attempt to send their messages for N ms and then shutdown if it still hasn't succeeded. Leaking the thread seems like a bug. Oh I think I understand the interpretation, the idea is that this is meant to attempt to close but then give up if the close doesn't complete in time. The problem is that this does actually close the producer but doesn't necessarily stop the thread and doesn't return any indication of what happened. - Jay --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review71595 --- On Dec. 29, 2014, 10:52 p.m., Parth Brahmbhatt wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated Dec. 29, 2014, 10:52 p.m.) Review request for kafka. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- KAFKA-1660: Adding tryClose(timeoutMillis) to producer. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java f61efb35db7e0de590556e6a94a7b5cb850cdae9 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 34624c3b7a1f28735ab6c63cc9e18a410e87e63c clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5baa6062bd9ba8a7d38058856ed2d831fae491f0 Diff: https://reviews.apache.org/r/29467/diff/ Testing --- existing unit tests passed. Thanks, Parth Brahmbhatt
Re: Review Request 29467: Patch for KAFKA-1660
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/ --- (Updated Dec. 29, 2014, 10:52 p.m.) Review request for kafka. Changes --- Adding tryClose(timeoutMillis) to producer. Bugs: KAFKA-1660 https://issues.apache.org/jira/browse/KAFKA-1660 Repository: kafka Description --- KAFKA-1660: Adding tryClose(timeoutMillis) to producer. Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java f61efb35db7e0de590556e6a94a7b5cb850cdae9 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 34624c3b7a1f28735ab6c63cc9e18a410e87e63c clients/src/main/java/org/apache/kafka/clients/producer/Producer.java 5baa6062bd9ba8a7d38058856ed2d831fae491f0 Diff: https://reviews.apache.org/r/29467/diff/ Testing (updated) --- existing unit tests passed. Thanks, Parth Brahmbhatt
