You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Guozhang Wang <gu...@linkedin.com> on 2014/02/14 00:10:02 UTC
Review Request 18102: Integration Test for New Producer: Part II
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description
-------
KAFKA-1260.Dummy
Diffs
-----
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Fix KAFKA-1260
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 27, 2014, 5:37 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Fix KAFKA-1260
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Add ProducerFailureHandlingTest
Dummy
Dummy
Dummy
Dummy
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java f43da80580f5a347f2e0435943ee2f0f829abc77
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/#review35593
-----------------------------------------------------------
Ship it!
Ship It!
- Jay Kreps
On Feb. 26, 2014, 9:51 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18102/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2014, 9:51 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1260
> https://issues.apache.org/jira/browse/KAFKA-1260
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Jun's comments.
>
> Incorporated Jay's fix for KAFKA-1266
>
> Fixed some minor issues in producer:
>
> 1. Close accumulator on shutdown
>
> 2. Return offset for ack=-1
>
> 3. Categorize error handling in send call
>
> Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
>
> Four test cases included in Part II:
>
> 1. testErrorInResponse.
>
> 2. testNoResponse.
>
> 3. testSendException.
>
> 4. testDeadBroker. Currently blocks..
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
> clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java f43da80580f5a347f2e0435943ee2f0f829abc77
> clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
> clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
> clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
> core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
> core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
>
> Diff: https://reviews.apache.org/r/18102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 26, 2014, 9:51 p.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Incorporated Jun's comments.
Incorporated Jay's fix for KAFKA-1266
Fixed some minor issues in producer:
1. Close accumulator on shutdown
2. Return offset for ack=-1
3. Categorize error handling in send call
Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
Four test cases included in Part II:
1. testErrorInResponse.
2. testNoResponse.
3. testSendException.
4. testDeadBroker. Currently blocks..
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java f43da80580f5a347f2e0435943ee2f0f829abc77
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 26, 2014, 9:49 p.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Dummy
Dummy
Dummy
Dummy
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java f43da80580f5a347f2e0435943ee2f0f829abc77
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Feb. 26, 2014, 7:56 p.m., Jun Rao wrote:
> > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala, lines 161-162
> > <https://reviews.apache.org/r/18102/diff/9/?file=504699#file504699line161>
> >
> > Shouldn't we add the code to call send after producer.close()?
The second part has been moved to the FailureHandling testsuite, I should have changed the comments.
> On Feb. 26, 2014, 7:56 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, lines 896-897
> > <https://reviews.apache.org/r/18102/diff/9/?file=504698#file504698line896>
> >
> > Could we change this to the following? Then we can use the name instead _1.
> >
> > for ( (topicPartition, responseStatus) <- ...
Done.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/#review35559
-----------------------------------------------------------
On Feb. 26, 2014, 9:49 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18102/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2014, 9:49 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1260
> https://issues.apache.org/jira/browse/KAFKA-1260
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Dummy
>
>
> Dummy
>
>
> Dummy
>
>
> Dummy
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
> clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java f43da80580f5a347f2e0435943ee2f0f829abc77
> clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
> clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
> clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
> core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
> core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
>
> Diff: https://reviews.apache.org/r/18102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/#review35559
-----------------------------------------------------------
Ship it!
+1 Just the following minor comments.
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/18102/#comment66164>
Could we change this to the following? Then we can use the name instead _1.
for ( (topicPartition, responseStatus) <- ...
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala
<https://reviews.apache.org/r/18102/#comment66162>
Shouldn't we add the code to call send after producer.close()?
- Jun Rao
On Feb. 26, 2014, 5:48 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18102/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2014, 5:48 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1260
> https://issues.apache.org/jira/browse/KAFKA-1260
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Jun's comments.
>
> Incorporated Jay's fix for KAFKA-1266
>
> Fixed some minor issues in producer:
>
> 1. Close accumulator on shutdown
>
> 2. Return offset for ack=-1
>
> 3. Categorize error handling in send call
>
> Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
>
> Four test cases included in Part II:
>
> 1. testErrorInResponse.
>
> 2. testNoResponse.
>
> 3. testSendException.
>
> 4. testDeadBroker. Currently blocks..
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
> clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java f43da80580f5a347f2e0435943ee2f0f829abc77
> clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
> clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
> clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
> core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
> core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
>
> Diff: https://reviews.apache.org/r/18102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 26, 2014, 5:48 p.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Incorporated Jun's comments.
Incorporated Jay's fix for KAFKA-1266
Fixed some minor issues in producer:
1. Close accumulator on shutdown
2. Return offset for ack=-1
3. Categorize error handling in send call
Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
Four test cases included in Part II:
1. testErrorInResponse.
2. testNoResponse.
3. testSendException.
4. testDeadBroker. Currently blocks..
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java f43da80580f5a347f2e0435943ee2f0f829abc77
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 26, 2014, 5:47 p.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Dummy
Dummy
Dummy
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java f43da80580f5a347f2e0435943ee2f0f829abc77
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Feb. 26, 2014, 2:47 a.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, lines 230-231
> > <https://reviews.apache.org/r/18102/diff/8/?file=504071#file504071line230>
> >
> > Not sure we need to wrap InterruptedException.
We need, since the accumulator.append can throw the runtime InterruptedException, which we do not want to throw outside KafkaProducer but instead wrap it with a checked KafkaException.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/#review35508
-----------------------------------------------------------
On Feb. 26, 2014, 1:46 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18102/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2014, 1:46 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1260
> https://issues.apache.org/jira/browse/KAFKA-1260
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Jay's fix for KAFKA-1266
>
> Fixed some minor issues in producer:
>
> 1. Close accumulator on shutdown
>
> 2. Return offset for ack=-1
>
> 3. Categorize error handling in send call
>
> Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
>
> Four test cases included in Part II:
>
> 1. testErrorInResponse.
>
> 2. testNoResponse.
>
> 3. testSendException.
>
> 4. testDeadBroker. Currently blocks..
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
> clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java f43da80580f5a347f2e0435943ee2f0f829abc77
> clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
> clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
> clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
> core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
> core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
>
> Diff: https://reviews.apache.org/r/18102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/#review35508
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
<https://reviews.apache.org/r/18102/#comment66079>
remove println.
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
<https://reviews.apache.org/r/18102/#comment66081>
Not sure we need to wrap InterruptedException.
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
<https://reviews.apache.org/r/18102/#comment66082>
Can we just let it pass through instead of rethrowing?
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java
<https://reviews.apache.org/r/18102/#comment66083>
Unused import.
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java
<https://reviews.apache.org/r/18102/#comment66084>
Do we need this comment? If so, let's add { } to the if clause.
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/18102/#comment66085>
We don't need acksPending to be in the constructor.
- Jun Rao
On Feb. 26, 2014, 1:46 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18102/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2014, 1:46 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1260
> https://issues.apache.org/jira/browse/KAFKA-1260
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Jay's fix for KAFKA-1266
>
> Fixed some minor issues in producer:
>
> 1. Close accumulator on shutdown
>
> 2. Return offset for ack=-1
>
> 3. Categorize error handling in send call
>
> Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
>
> Four test cases included in Part II:
>
> 1. testErrorInResponse.
>
> 2. testNoResponse.
>
> 3. testSendException.
>
> 4. testDeadBroker. Currently blocks..
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
> clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java f43da80580f5a347f2e0435943ee2f0f829abc77
> clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
> clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
> clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
> core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
> core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
>
> Diff: https://reviews.apache.org/r/18102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 26, 2014, 1:46 a.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Incorporated Jay's fix for KAFKA-1266
Fixed some minor issues in producer:
1. Close accumulator on shutdown
2. Return offset for ack=-1
3. Categorize error handling in send call
Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
Four test cases included in Part II:
1. testErrorInResponse.
2. testNoResponse.
3. testSendException.
4. testDeadBroker. Currently blocks..
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java f43da80580f5a347f2e0435943ee2f0f829abc77
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 26, 2014, 1:46 a.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Dummy
Dummy
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java f43da80580f5a347f2e0435943ee2f0f829abc77
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 26, 2014, 12:59 a.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Incorporated Jay's fix for KAFKA-1266
Fixed some minor issues in producer:
1. Close accumulator on shutdown
2. Return offset for ack=-1
3. Categorize error handling in send call
Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
Four test cases included in Part II:
1. testErrorInResponse.
2. testNoResponse.
3. testSendException.
4. testDeadBroker. Currently blocks..
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 26, 2014, 12:58 a.m.)
Review request for kafka.
Summary (updated)
-----------------
Integration Test for New Producer: Part II
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Dummy
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: New Producer Integration Test Phase II
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/#review35468
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java
<https://reviews.apache.org/r/18102/#comment66026>
it seems this constructor now is only used by the MockProducer. It is slightly awkward to have both these constructors. With the new one, can we make this one private and change all usages to point to the new constructor instead?
core/src/main/scala/kafka/api/ProducerResponse.scala
<https://reviews.apache.org/r/18102/#comment65999>
Can this be DelayedProducerResponseStatus along the lines of ProducerResponseStatus. Also I'm not quite sure I understand the changes related to DelayedProduceResponseStatus. Let's chat about this.
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment66030>
I'm trying to understand the scope of this test. It says failure handling and I'm not sure where you want to put the tests for testing the various ack modes in the presence of broker failures. One test I can think of is to have the producer send data using ack = -1 and validate the offsets returned and actual data to verify that there is no data loss.
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment66005>
not-exist-topic -> non existing topics?
cased -> caused
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment66006>
cased -> caused
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment66007>
that can be thrown
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment66012>
cased -> caused
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment66014>
Instead of too large, can we say sending a message >= bufferSize will cause the producer to throw a BufferExhaustedException?
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment66013>
ditto
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment66021>
is this TODO still required?
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment66023>
can we remove this?
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment66024>
ditto here
- Neha Narkhede
On Feb. 20, 2014, 11:45 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18102/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 11:45 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1260
> https://issues.apache.org/jira/browse/KAFKA-1260
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Rebased on producer re-try.
>
> Incorporated Jay's fix for KAFKA-1266
>
> Fixed some minor issues in producer:
>
> 1. Close accumulator on shutdown
>
> 2. Return offset for ack=-1
>
> 3. Categorize error handling in send call
>
> Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
>
> Four test cases included in Part II:
>
> 1. testErrorInResponse.
>
> 2. testNoResponse.
>
> 3. testSendException.
>
> 4. testDeadBroker. Currently blocks..
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
> clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
> clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
> clients/src/main/java/org/apache/kafka/common/errors/FatalException.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java 737b7f07b16a0d6549e835c33c9dfad58008a590
> clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
> core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
> core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
>
> Diff: https://reviews.apache.org/r/18102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 18102: New Producer Integration Test Phase II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/#review35418
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java
<https://reviews.apache.org/r/18102/#comment65901>
Not removing it :) Just make them in one line.
- Guozhang Wang
On Feb. 20, 2014, 11:45 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18102/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 11:45 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1260
> https://issues.apache.org/jira/browse/KAFKA-1260
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Rebased on producer re-try.
>
> Incorporated Jay's fix for KAFKA-1266
>
> Fixed some minor issues in producer:
>
> 1. Close accumulator on shutdown
>
> 2. Return offset for ack=-1
>
> 3. Categorize error handling in send call
>
> Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
>
> Four test cases included in Part II:
>
> 1. testErrorInResponse.
>
> 2. testNoResponse.
>
> 3. testSendException.
>
> 4. testDeadBroker. Currently blocks..
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
> clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
> clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
> clients/src/main/java/org/apache/kafka/common/errors/FatalException.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java 737b7f07b16a0d6549e835c33c9dfad58008a590
> clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
> core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
> core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
>
> Diff: https://reviews.apache.org/r/18102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 18102: New Producer Integration Test Phase II
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Feb. 25, 2014, 4:39 a.m., Jay Kreps wrote:
> > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala, line 163
> > <https://reviews.apache.org/r/18102/diff/6/?file=499586#file499586line163>
> >
> > It might be simpler to do something like
> > if(producer1 != null) producer1.close()
> > in the tearDown() method of the test to avoid the nested try/catch.
Originally I did this, but later realized that for some tests close() will block. I think probably I will just made it to null for now.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/#review35370
-----------------------------------------------------------
On Feb. 20, 2014, 11:45 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18102/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 11:45 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1260
> https://issues.apache.org/jira/browse/KAFKA-1260
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Rebased on producer re-try.
>
> Incorporated Jay's fix for KAFKA-1266
>
> Fixed some minor issues in producer:
>
> 1. Close accumulator on shutdown
>
> 2. Return offset for ack=-1
>
> 3. Categorize error handling in send call
>
> Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
>
> Four test cases included in Part II:
>
> 1. testErrorInResponse.
>
> 2. testNoResponse.
>
> 3. testSendException.
>
> 4. testDeadBroker. Currently blocks..
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
> clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
> clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
> clients/src/main/java/org/apache/kafka/common/errors/FatalException.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java 737b7f07b16a0d6549e835c33c9dfad58008a590
> clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
> core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
> core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
>
> Diff: https://reviews.apache.org/r/18102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 18102: New Producer Integration Test Phase II
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Feb. 25, 2014, 4:39 a.m., Jay Kreps wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 222
> > <https://reviews.apache.org/r/18102/diff/6/?file=499574#file499574line222>
> >
> > Why are we using instanceof instead of catching the individual exception types?
> >
> > Also why are we wrapping RuntimeExceptions in KafkaException?
> >
> > Basically I don't think I follow the rationale here.
Originally the send call does not throw any exceptions, all errors are wrapped in the future returned. We decided to only wrap ApiExceptions but throw others in send call, so I think we either 1) declare the runtime throwable exceptions in the function signature, or 2) wrap them in checked KafkaException.
> On Feb. 25, 2014, 4:39 a.m., Jay Kreps wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 267
> > <https://reviews.apache.org/r/18102/diff/6/?file=499574#file499574line267>
> >
> > You seem to have removed the close on the accumulator is this an accident?
Not an accident :) I realized that accumulator.close() is called in initiateClose already.
> On Feb. 25, 2014, 4:39 a.m., Jay Kreps wrote:
> > clients/src/main/java/org/apache/kafka/common/errors/FatalException.java, line 1
> > <https://reviews.apache.org/r/18102/diff/6/?file=499580#file499580line1>
> >
> > The current logic is that all exceptions that don't implement RetriableException are fatal so I don't think we need this. Originally I had thought to use RetriableException to mark an idempotent exception but the current usage is actually for any non-fatal exception. The rationale was (1) We shouldn't end up accidentally retrying if we through an unexpected error (out of memory or null pointer or something) and (2) when we have the idempotent producer retry will always be idempotent so modeling that in the exception hiearchy is premature.
Ack
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/#review35370
-----------------------------------------------------------
On Feb. 20, 2014, 11:45 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18102/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 11:45 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1260
> https://issues.apache.org/jira/browse/KAFKA-1260
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Rebased on producer re-try.
>
> Incorporated Jay's fix for KAFKA-1266
>
> Fixed some minor issues in producer:
>
> 1. Close accumulator on shutdown
>
> 2. Return offset for ack=-1
>
> 3. Categorize error handling in send call
>
> Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
>
> Four test cases included in Part II:
>
> 1. testErrorInResponse.
>
> 2. testNoResponse.
>
> 3. testSendException.
>
> 4. testDeadBroker. Currently blocks..
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
> clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
> clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
> clients/src/main/java/org/apache/kafka/common/errors/FatalException.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java 737b7f07b16a0d6549e835c33c9dfad58008a590
> clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
> core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
> core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
>
> Diff: https://reviews.apache.org/r/18102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 18102: New Producer Integration Test Phase II
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/#review35370
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
<https://reviews.apache.org/r/18102/#comment65847>
Why are we using instanceof instead of catching the individual exception types?
Also why are we wrapping RuntimeExceptions in KafkaException?
Basically I don't think I follow the rationale here.
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
<https://reviews.apache.org/r/18102/#comment65848>
You seem to have removed the close on the accumulator is this an accident?
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java
<https://reviews.apache.org/r/18102/#comment65849>
Hmm so we need to make a new atomic reference now? This seems much much worse then just recreating the RecordMetadata instance, no?
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java
<https://reviews.apache.org/r/18102/#comment65850>
I'm not sold that this refactor actually made things better. Now everything goes through future.get which is needlessly synchronized, right?
clients/src/main/java/org/apache/kafka/common/errors/FatalException.java
<https://reviews.apache.org/r/18102/#comment65851>
The current logic is that all exceptions that don't implement RetriableException are fatal so I don't think we need this. Originally I had thought to use RetriableException to mark an idempotent exception but the current usage is actually for any non-fatal exception. The rationale was (1) We shouldn't end up accidentally retrying if we through an unexpected error (out of memory or null pointer or something) and (2) when we have the idempotent producer retry will always be idempotent so modeling that in the exception hiearchy is premature.
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java
<https://reviews.apache.org/r/18102/#comment65852>
How come you are removing this? Is it not an exception that can occur on the server?
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/18102/#comment65854>
Not sure I grok the changes here, may need to chat about it...
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment65855>
This package needs to match the directory. Same with the other integration test. You have it in kafka.api.* but declare kafka.test.*
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment65862>
Does it make sense to combine these all? wouldn't it be easier to just have 4 test cases?
That is usually easier to reason about and understand when they fail.
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment65856>
Can you make a method for this. It can be a local method for now.
def makeProducer(brokers, acks, fetchTimeout) or whatever...
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment65858>
There is no need to catch the exception and fail, an uncaught exception will always fail the test.
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment65859>
If you expect a particular exception use the scalatest utility intercept. It looks like this:
intercept[ExecutionException]{
// do something that should throw that exception
}
Much easier to read.
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment65860>
Ditto
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment65861>
Ditto
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment65857>
It might be simpler to do something like
if(producer1 != null) producer1.close()
in the tearDown() method of the test to avoid the nested try/catch.
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment65863>
Would it make sense to break these into separate test methods?
- Jay Kreps
On Feb. 20, 2014, 11:45 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18102/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 11:45 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1260
> https://issues.apache.org/jira/browse/KAFKA-1260
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Rebased on producer re-try.
>
> Incorporated Jay's fix for KAFKA-1266
>
> Fixed some minor issues in producer:
>
> 1. Close accumulator on shutdown
>
> 2. Return offset for ack=-1
>
> 3. Categorize error handling in send call
>
> Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
>
> Four test cases included in Part II:
>
> 1. testErrorInResponse.
>
> 2. testNoResponse.
>
> 3. testSendException.
>
> 4. testDeadBroker. Currently blocks..
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
> clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
> clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
> clients/src/main/java/org/apache/kafka/common/errors/FatalException.java PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java 737b7f07b16a0d6549e835c33c9dfad58008a590
> clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
> core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
> core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
>
> Diff: https://reviews.apache.org/r/18102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 18102: New Producer Integration Test Phase II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 20, 2014, 11:45 p.m.)
Review request for kafka.
Summary (updated)
-----------------
New Producer Integration Test Phase II
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Rebased on producer re-try.
Incorporated Jay's fix for KAFKA-1266
Fixed some minor issues in producer:
1. Close accumulator on shutdown
2. Return offset for ack=-1
3. Categorize error handling in send call
Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
Four test cases included in Part II:
1. testErrorInResponse.
2. testNoResponse.
3. testSendException.
4. testDeadBroker. Currently blocks..
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/errors/FatalException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java 737b7f07b16a0d6549e835c33c9dfad58008a590
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Fix KAFKA-1235
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 20, 2014, 11:45 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Fix KAFKA-1235
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Dummy
Dummy
Dummy 3
FIXED!
Dummy 2
Dummy
KAFKA-1260.v1
KAFKA-1260.Dummy
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/errors/FatalException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java 737b7f07b16a0d6549e835c33c9dfad58008a590
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: New Producer Integration Test: Phase II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 20, 2014, 11:34 p.m.)
Review request for kafka.
Summary (updated)
-----------------
New Producer Integration Test: Phase II
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description
-------
Rebased on producer re-try.
Incorporated Jay's fix for KAFKA-1266
Fixed some minor issues in producer:
1. Close accumulator on shutdown
2. Return offset for ack=-1
3. Categorize error handling in send call
Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
Four test cases included in Part II:
1. testErrorInResponse.
2. testNoResponse.
3. testSendException.
4. testDeadBroker. Currently blocks..
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/errors/FatalException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java 737b7f07b16a0d6549e835c33c9dfad58008a590
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
clients/src/test/java/org/apache/kafka/test/TestUtils.java 36cfc0fda742eb501af2c2c0330e3f461cf1f40c
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Fix KAFKA-1235
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 20, 2014, 11:28 p.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Rebased on producer re-try.
Incorporated Jay's fix for KAFKA-1266
Fixed some minor issues in producer:
1. Close accumulator on shutdown
2. Return offset for ack=-1
3. Categorize error handling in send call
Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
Four test cases included in Part II:
1. testErrorInResponse.
2. testNoResponse.
3. testSendException.
4. testDeadBroker. Currently blocks..
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/errors/FatalException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java 737b7f07b16a0d6549e835c33c9dfad58008a590
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
clients/src/test/java/org/apache/kafka/test/TestUtils.java 36cfc0fda742eb501af2c2c0330e3f461cf1f40c
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Fix KAFKA-1235
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 20, 2014, 11:27 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Fix KAFKA-1235
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Dummy
Dummy 3
FIXED!
Dummy 2
Dummy
KAFKA-1260.v1
KAFKA-1260.Dummy
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd
clients/src/main/java/org/apache/kafka/common/errors/FatalException.java PRE-CREATION
clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java 737b7f07b16a0d6549e835c33c9dfad58008a590
clients/src/main/java/org/apache/kafka/common/network/Selector.java f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
clients/src/test/java/org/apache/kafka/test/TestUtils.java 36cfc0fda742eb501af2c2c0330e3f461cf1f40c
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 19, 2014, 9:51 p.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Rebased on producer re-try.
Fixed some minor issues in producer:
1. Close accumulator on shutdown
2. Return offset for ack=-1
3. Categorize error handling in send call
Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255.
Four test cases included in Part II:
1. testErrorInResponse.
2. testNoResponse. Currently blocks..
3. testSendException.
4. testDeadBroker. Currently blocks..
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java 737b7f07b16a0d6549e835c33c9dfad58008a590
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
clients/src/test/java/org/apache/kafka/test/TestUtils.java 36cfc0fda742eb501af2c2c0330e3f461cf1f40c
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 19, 2014, 9:49 p.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Dummy 3
FIXED!
Dummy 2
Dummy
KAFKA-1260.v1
KAFKA-1260.Dummy
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java e4bc97279585818860487a39a93b6481742b91db
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 62613a3e29a7e5ffb1cc56d267793fef72857fc6
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java eb16f6d236e07b16654623606294a051531b5f58
clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java 737b7f07b16a0d6549e835c33c9dfad58008a590
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
clients/src/test/java/org/apache/kafka/test/TestUtils.java 36cfc0fda742eb501af2c2c0330e3f461cf1f40c
core/src/main/scala/kafka/api/ProducerResponse.scala 06261b9136399e48a8cb75f1c88a4cbfd1217de4
core/src/main/scala/kafka/server/KafkaApis.scala ae2df2014a08aaa95b5eaa430684cfdb79d4f55e
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 14, 2014, 11:02 p.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
A Dummy update for Jay to trouble shoot KAFKA-1266
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 3d180e885a25fa6b138f544ac320ec9e0d2a1e7f
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 52d30a86d04393ec06e8b362e91f31492a278680
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java 7a440a3dd29c704dac9087fdac28c35d3e33e345
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java d93a455827a6747e02fdb388e34320f648877c34
clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java ce95ca04aa842078ad20ea3ae2c764b51ac76f7a
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
clients/src/test/java/org/apache/kafka/test/TestUtils.java 36cfc0fda742eb501af2c2c0330e3f461cf1f40c
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 14, 2014, 11 p.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Dummy
KAFKA-1260.v1
KAFKA-1260.Dummy
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 3d180e885a25fa6b138f544ac320ec9e0d2a1e7f
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 52d30a86d04393ec06e8b362e91f31492a278680
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java 7a440a3dd29c704dac9087fdac28c35d3e33e345
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java d93a455827a6747e02fdb388e34320f648877c34
clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java ce95ca04aa842078ad20ea3ae2c764b51ac76f7a
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
clients/src/test/java/org/apache/kafka/test/TestUtils.java 36cfc0fda742eb501af2c2c0330e3f461cf1f40c
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote:
> > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala, line 67
> > <https://reviews.apache.org/r/18102/diff/2/?file=484693#file484693line67>
> >
> > You should be able to create this now. There is a TestUtil in the clients package. Either way cut and pasting each config three times isn't right...
The problem is that there are two TestUtils one in core and one in clients, so we cannot import both.
> On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote:
> > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala, line 36
> > <https://reviews.apache.org/r/18102/diff/2/?file=484693#file484693line36>
> >
> > Is there a reason not to combine this with the other producer test?
I was trying to create more than one test suite for producer, each on different functionality. Another reason would just to not make the test file too big..
> On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote:
> > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala, line 18
> > <https://reviews.apache.org/r/18102/diff/2/?file=484693#file484693line18>
> >
> > The package doesn't match the directory.
Are we enforcing the match? I am not aware of this since the old test package does not enforce the match either.
> On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 221
> > <https://reviews.apache.org/r/18102/diff/2/?file=484686#file484686line221>
> >
> > We shouldn't use instanceof we should use separate catch clauses for each exception type, right?
> >
> > Also this isn't quite the logic we discussed. Shouldn't it be the case that we only catch ApiException?
I thought the plan is to that send() can throw KafkaException, and future will contain ApiException.
> On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote:
> > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala, line 129
> > <https://reviews.apache.org/r/18102/diff/2/?file=484693#file484693line129>
> >
> > This would probably make more sense as two tests, no?
I think this is a small enough test that we can wrap in one test case.
> On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java, line 66
> > <https://reviews.apache.org/r/18102/diff/2/?file=484688#file484688line66>
> >
> > Is this thread safe? Is the only goal to avoid double-creating the RecordMetadata object? If so I wonder if this is actually better. Object creation is very fast and few people would end up using both the get() and the callback at the same time.
> >
> > The cleanup I was imagining was to explore the possibility of removing FutureRecordmetadata and replacing it with a generic SettableFuture and then setting the RecordMetadata instance on this future. Not sure if this would be better either...
Yeah it is not thread safe. We can use an atomic reference to fix this.
Another reason I just want one place to create the object is to avoid two places doing the same logic, since if we change one place in the future we have to remember to change the other (paranoid live at last)...
Now after give it a second thought I am open to either the old way or the atomic reference. Not sure if the general SettableFuture works here?
> On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java, line 82
> > <https://reviews.apache.org/r/18102/diff/2/?file=484689#file484689line82>
> >
> > Does this mean reqacuiring the countdownlatch?
Yes, and if the countdown is already zero it should pass immediately.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/#review34437
-----------------------------------------------------------
On Feb. 13, 2014, 11:16 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18102/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2014, 11:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1260
> https://issues.apache.org/jira/browse/KAFKA-1260
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fixed some minor issues in producer:
>
> 1. Close accumulator on shutdown
>
> 2. Return offset for ack=-1
>
> 3. Categorize error handling in send call
>
> Four test cases included in Part II:
>
> 1. testErrorInResponse.
>
> 2. testNoResponse. Currently blocks..
>
> 3. testSendException.
>
> 4. testDeadBroker. Currently failes
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 3d180e885a25fa6b138f544ac320ec9e0d2a1e7f
> clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
> clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java 7a440a3dd29c704dac9087fdac28c35d3e33e345
> clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java ce95ca04aa842078ad20ea3ae2c764b51ac76f7a
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
> clients/src/test/java/org/apache/kafka/test/TestUtils.java 36cfc0fda742eb501af2c2c0330e3f461cf1f40c
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
>
> Diff: https://reviews.apache.org/r/18102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/#review34437
-----------------------------------------------------------
Is there a case for failure and recovery. The scenario I am most interested in is:
1. bring up 2+ brokers
2. produce data to a topic with one partition
3. kill leader for that partition
4. produce data again (should fail since broker is down but we should initiate metadata refresh)
5. produce data again (should succeed on new leader)
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
<https://reviews.apache.org/r/18102/#comment64532>
We shouldn't use instanceof we should use separate catch clauses for each exception type, right?
Also this isn't quite the logic we discussed. Shouldn't it be the case that we only catch ApiException?
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java
<https://reviews.apache.org/r/18102/#comment64633>
Is this thread safe? Is the only goal to avoid double-creating the RecordMetadata object? If so I wonder if this is actually better. Object creation is very fast and few people would end up using both the get() and the callback at the same time.
The cleanup I was imagining was to explore the possibility of removing FutureRecordmetadata and replacing it with a generic SettableFuture and then setting the RecordMetadata instance on this future. Not sure if this would be better either...
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java
<https://reviews.apache.org/r/18102/#comment64637>
Does this mean reqacuiring the countdownlatch?
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment64638>
The package doesn't match the directory.
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment64639>
Is there a reason not to combine this with the other producer test?
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment64640>
You should be able to create this now. There is a TestUtil in the clients package. Either way cut and pasting each config three times isn't right...
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/18102/#comment64642>
This would probably make more sense as two tests, no?
- Jay Kreps
On Feb. 13, 2014, 11:16 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18102/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2014, 11:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1260
> https://issues.apache.org/jira/browse/KAFKA-1260
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fixed some minor issues in producer:
>
> 1. Close accumulator on shutdown
>
> 2. Return offset for ack=-1
>
> 3. Categorize error handling in send call
>
> Four test cases included in Part II:
>
> 1. testErrorInResponse.
>
> 2. testNoResponse. Currently blocks..
>
> 3. testSendException.
>
> 4. testDeadBroker. Currently failes
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 3d180e885a25fa6b138f544ac320ec9e0d2a1e7f
> clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
> clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java 7a440a3dd29c704dac9087fdac28c35d3e33e345
> clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java ce95ca04aa842078ad20ea3ae2c764b51ac76f7a
> clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
> clients/src/test/java/org/apache/kafka/test/TestUtils.java 36cfc0fda742eb501af2c2c0330e3f461cf1f40c
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
>
> Diff: https://reviews.apache.org/r/18102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 13, 2014, 11:16 p.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Fixed some minor issues in producer:
1. Close accumulator on shutdown
2. Return offset for ack=-1
3. Categorize error handling in send call
Four test cases included in Part II:
1. testErrorInResponse.
2. testNoResponse. Currently blocks..
3. testSendException.
4. testDeadBroker. Currently failes
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 3d180e885a25fa6b138f544ac320ec9e0d2a1e7f
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java 7a440a3dd29c704dac9087fdac28c35d3e33e345
clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java ce95ca04aa842078ad20ea3ae2c764b51ac76f7a
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
clients/src/test/java/org/apache/kafka/test/TestUtils.java 36cfc0fda742eb501af2c2c0330e3f461cf1f40c
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 13, 2014, 11:14 p.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
KAFKA-1260.v1
KAFKA-1260.Dummy
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 3d180e885a25fa6b138f544ac320ec9e0d2a1e7f
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java 8c776980ef1f5167fb02dda36f6ad2385bd62bbd
clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java 22d4c79bc06fb77af30ab930855632c402c5a72e
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java 7a440a3dd29c704dac9087fdac28c35d3e33e345
clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java ce95ca04aa842078ad20ea3ae2c764b51ac76f7a
clients/src/main/java/org/apache/kafka/common/protocol/Errors.java f88992a0cafd9639b4c5823aa80de1daf8b0eadd
clients/src/test/java/org/apache/kafka/test/TestUtils.java 36cfc0fda742eb501af2c2c0330e3f461cf1f40c
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e
core/src/test/scala/unit/kafka/utils/TestUtils.scala 1c7a450651978e121376c226987b0d835f395a2a
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 18102: Integration Test for New Producer: Part II
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18102/
-----------------------------------------------------------
(Updated Feb. 13, 2014, 11:13 p.m.)
Review request for kafka.
Bugs: KAFKA-1260
https://issues.apache.org/jira/browse/KAFKA-1260
Repository: kafka
Description (updated)
-------
Fixed some minor issues in producer:
1. Close accumulator on shutdown
2. Return offset for ack=-1
Four test cases included in Part II:
1. testErrorInResponse.
2. testNoResponse. Currently blocks..
3. testSendException.
4. testDeadBroker. Currently failes
Diffs
-----
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/18102/diff/
Testing
-------
Thanks,
Guozhang Wang