You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Paul Pearcy <pp...@gmail.com> on 2015/01/22 08:04:13 UTC
Review Request 30158: Patch for KAFKA-1835
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30158/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1835
https://issues.apache.org/jira/browse/KAFKA-1835
Repository: kafka
Description
-------
KAFKA-1835 - New producer updates to make blocking behavior explicit
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java fc71710dd5997576d3841a1c3b0f7e19a8c9698e
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java 8b3e565edd1ae04d8d34bd9f1a41e9fa8c880a75
core/src/test/scala/integration/kafka/api/ProducerBlockingTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/utils/TestUtils.scala ac15d34425795d5be20c51b01fa1108bdcd66583
Diff: https://reviews.apache.org/r/30158/diff/
Testing
-------
Thanks,
Paul Pearcy
Re: Review Request 30158: Patch for KAFKA-1835
Posted by Paul Pearcy <pp...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30158/#review69138
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
<https://reviews.apache.org/r/30158/#comment113759>
Here are some comments on this file from Ewan:
* No need to use the object forms of primitive types, change Boolean -> boolean, Long -> long, etc.
* initialized should be an AtomicBoolean or volatile boolean since it's read/written from different threads
* Error handling when waiting for the Future to finish seems wrong -- if there is an exception, we probably want to pass it along/throw another one to indicate the problem to the caller. Currently it just falls through and then only throws an exception when send() is called, so the error ends up disconnected from the source of the problem. It seems like it would be better to just handle the error immediately.
* Similarly, I don't think send() should check initialized if preinitialization is handled in the constructor -- if failure to preinitialize also threw an exception, then it would be impossible to call send() unless preinitialization was complete.
* If you follow the above approach, you can avoid making initialized a field in the class. It would only need to be a local variable since it would only be used in the constructor.
* Do we even need the ExecutorService? Since the thread creating the producer is going to block by calling Future.get(), what does having the executor accomplish?
* initializeProducer() doesn't need a return value since only ever returns true.
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java
<https://reviews.apache.org/r/30158/#comment113760>
Comments from Ewan:
* Config has a getList() method and ConfigDef has a LIST type. Use those for pre.initialize.topics instead of parsing the list yourself.
* I think the docstrings could be better, e.g.:
pre.initialize.topics: "List of topics to preload metadata for when creating the producer so subsequent calls to send are guaranteed not to block. If metadata for these topics cannot be loaded within <code>pre.initialize.timeout.ms</code> milliseconds, the producer constructor will throw an exception."
pre.initialize.timeout.ms: "The producer blocks when sending the first message to a topic if metadata is not yet available for that topic. When this configuration is greater than 0, metadata for the topics specified by <code>pre.initialize.topics</code> are prefetched during construction, throwing an exception after <code>pre.initialize.timeout.ms</code> milliseconds if the metadata has not been populated."
- Paul Pearcy
On Jan. 22, 2015, 7:04 a.m., Paul Pearcy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30158/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2015, 7:04 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1835
> https://issues.apache.org/jira/browse/KAFKA-1835
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1835 - New producer updates to make blocking behavior explicit
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java fc71710dd5997576d3841a1c3b0f7e19a8c9698e
> clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java 8b3e565edd1ae04d8d34bd9f1a41e9fa8c880a75
> core/src/test/scala/integration/kafka/api/ProducerBlockingTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/utils/TestUtils.scala ac15d34425795d5be20c51b01fa1108bdcd66583
>
> Diff: https://reviews.apache.org/r/30158/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Paul Pearcy
>
>
Re: Review Request 30158: Patch for KAFKA-1835
Posted by Steven Wu <st...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30158/#review69515
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
<https://reviews.apache.org/r/30158/#comment114212>
I would
- for-loop to call Metadata.add(topic). this way we add all topics to Metadata
- call Metadata#requestUpdate() to tigger Sender thread to request update for all listed topics
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
<https://reviews.apache.org/r/30158/#comment114215>
I would not use initialized flag, as long as we fix KafkaProducer#waitOnMetadata to allow value 0 for non-blocking.
- Steven Wu
On Jan. 22, 2015, 7:04 a.m., Paul Pearcy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30158/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2015, 7:04 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1835
> https://issues.apache.org/jira/browse/KAFKA-1835
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1835 - New producer updates to make blocking behavior explicit
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java fc71710dd5997576d3841a1c3b0f7e19a8c9698e
> clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java 8b3e565edd1ae04d8d34bd9f1a41e9fa8c880a75
> core/src/test/scala/integration/kafka/api/ProducerBlockingTest.scala PRE-CREATION
> core/src/test/scala/unit/kafka/utils/TestUtils.scala ac15d34425795d5be20c51b01fa1108bdcd66583
>
> Diff: https://reviews.apache.org/r/30158/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Paul Pearcy
>
>