You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jay Kreps <bo...@gmail.com> on 2015/02/09 02:27:58 UTC
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
Posted by Jay Kreps <bo...@gmail.com>.
> 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
>
>