You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jay Kreps <bo...@gmail.com> on 2014/02/18 00:25:17 UTC

Review Request 18199: Patch for KAFKA-1252

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18199/
-----------------------------------------------------------

Review request for kafka.


Bugs: KAFKA-1252
    https://issues.apache.org/jira/browse/KAFKA-1252


Repository: kafka


Description
-------

KAFKA-1252 Implement retries in new producer.


Diffs
-----

  clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 3d180e885a25fa6b138f544ac320ec9e0d2a1e7f 
  clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java dca9802c8d0918cf5e284ff5b5ea2ccdf788602b 
  clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 52d30a86d04393ec06e8b362e91f31492a278680 
  clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java be8a4a399b8b43beabe2c34f8f4f728cb63a29bf 
  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/Cluster.java c17a8f8162db9386a36d4204b9036676344a3451 
  clients/src/main/java/org/apache/kafka/common/errors/CorruptRecordException.java 673f61d6271c55fe7f3422e49a3f1f8f7d3d2206 
  clients/src/main/java/org/apache/kafka/common/errors/LeaderNotAvailableException.java 0bde6b5a351fc2475181b927effdd5ee8c7ff85c 
  clients/src/main/java/org/apache/kafka/common/errors/NetworkException.java 3a041593d76cfb52471c49ca5a37ebf6129d8131 
  clients/src/main/java/org/apache/kafka/common/errors/NotLeaderForPartitionException.java 5adc72ccf2d0cf4c0503df8d19c945a2adb2df90 
  clients/src/main/java/org/apache/kafka/common/errors/OffsetOutOfRangeException.java d01698a3efca7d4a9ee9b20f6f56abe2f299de06 
  clients/src/main/java/org/apache/kafka/common/errors/RetriableException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/errors/RetryableException.java c7f2f222f712a8b6659599c25fffc4fd55772d40 
  clients/src/main/java/org/apache/kafka/common/errors/TimeoutException.java dffd64d19c35aa1f0378d220dedf619a9c115b6d 
  clients/src/main/java/org/apache/kafka/common/errors/UnknownTopicOrPartitionException.java 73d1953cbe045d7f67e423ff2f0f4d27465db41d 
  clients/src/main/java/org/apache/kafka/common/network/Selector.java 8ed4c73146b2e7d8bb0bb1779dd6f942b231a627 
  clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java 1bbe83c1bfd7599dbba12424deee302e62c7a2b5 
  clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java 41c028bffbda1c418d827a1f6a8d2a63cdf3e373 

Diff: https://reviews.apache.org/r/18199/diff/


Testing
-------


Thanks,

Jay Kreps

Re: Review Request 18199: Patch for KAFKA-1252

Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18199/#review34795
-----------------------------------------------------------

Ship it!


Minor comments below


clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java
<https://reviews.apache.org/r/18199/#comment65112>

    MAX_NUM_RETRIES_CONFIG?



clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
<https://reviews.apache.org/r/18199/#comment65125>

    This comment is not accurate, we will only return the error to user if cannot retry.



clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
<https://reviews.apache.org/r/18199/#comment65130>

    Can we also use class hierarchy here like canRetry does instead of checking individual exception code? By doing so we move the checking definitions to the exceptions themselves instead of putting them here.



clients/src/main/java/org/apache/kafka/common/errors/RetriableException.java
<https://reviews.apache.org/r/18199/#comment65131>

    Just thought about RecordTooLargeException, for now it may be inherited from some FatalException?


- Guozhang Wang


On Feb. 17, 2014, 11:25 p.m., Jay Kreps wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18199/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2014, 11:25 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1252
>     https://issues.apache.org/jira/browse/KAFKA-1252
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1252 Implement retries in new producer.
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 3d180e885a25fa6b138f544ac320ec9e0d2a1e7f 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java dca9802c8d0918cf5e284ff5b5ea2ccdf788602b 
>   clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 52d30a86d04393ec06e8b362e91f31492a278680 
>   clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java be8a4a399b8b43beabe2c34f8f4f728cb63a29bf 
>   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/Cluster.java c17a8f8162db9386a36d4204b9036676344a3451 
>   clients/src/main/java/org/apache/kafka/common/errors/CorruptRecordException.java 673f61d6271c55fe7f3422e49a3f1f8f7d3d2206 
>   clients/src/main/java/org/apache/kafka/common/errors/LeaderNotAvailableException.java 0bde6b5a351fc2475181b927effdd5ee8c7ff85c 
>   clients/src/main/java/org/apache/kafka/common/errors/NetworkException.java 3a041593d76cfb52471c49ca5a37ebf6129d8131 
>   clients/src/main/java/org/apache/kafka/common/errors/NotLeaderForPartitionException.java 5adc72ccf2d0cf4c0503df8d19c945a2adb2df90 
>   clients/src/main/java/org/apache/kafka/common/errors/OffsetOutOfRangeException.java d01698a3efca7d4a9ee9b20f6f56abe2f299de06 
>   clients/src/main/java/org/apache/kafka/common/errors/RetriableException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/RetryableException.java c7f2f222f712a8b6659599c25fffc4fd55772d40 
>   clients/src/main/java/org/apache/kafka/common/errors/TimeoutException.java dffd64d19c35aa1f0378d220dedf619a9c115b6d 
>   clients/src/main/java/org/apache/kafka/common/errors/UnknownTopicOrPartitionException.java 73d1953cbe045d7f67e423ff2f0f4d27465db41d 
>   clients/src/main/java/org/apache/kafka/common/network/Selector.java 8ed4c73146b2e7d8bb0bb1779dd6f942b231a627 
>   clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java 1bbe83c1bfd7599dbba12424deee302e62c7a2b5 
>   clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java 41c028bffbda1c418d827a1f6a8d2a63cdf3e373 
> 
> Diff: https://reviews.apache.org/r/18199/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jay Kreps
> 
>

Re: Review Request 18199: Patch for KAFKA-1252

Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18199/#review34981
-----------------------------------------------------------



clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
<https://reviews.apache.org/r/18199/#comment65375>

    Should we just pass in 1, instead i + 1 to metadataNodeIndex()?



clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
<https://reviews.apache.org/r/18199/#comment65374>

    Should we just pass in 1, instead i + 1 to metadataNodeIndex()?


- Jun Rao


On Feb. 19, 2014, 1:15 a.m., Jay Kreps wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18199/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2014, 1:15 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1252
>     https://issues.apache.org/jira/browse/KAFKA-1252
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1252 Implement retries in new producer.
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 3d180e885a25fa6b138f544ac320ec9e0d2a1e7f 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java dca9802c8d0918cf5e284ff5b5ea2ccdf788602b 
>   clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 52d30a86d04393ec06e8b362e91f31492a278680 
>   clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java be8a4a399b8b43beabe2c34f8f4f728cb63a29bf 
>   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/Cluster.java c17a8f8162db9386a36d4204b9036676344a3451 
>   clients/src/main/java/org/apache/kafka/common/errors/CorruptRecordException.java 673f61d6271c55fe7f3422e49a3f1f8f7d3d2206 
>   clients/src/main/java/org/apache/kafka/common/errors/InvalidMetadataException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/LeaderNotAvailableException.java 0bde6b5a351fc2475181b927effdd5ee8c7ff85c 
>   clients/src/main/java/org/apache/kafka/common/errors/NetworkException.java 3a041593d76cfb52471c49ca5a37ebf6129d8131 
>   clients/src/main/java/org/apache/kafka/common/errors/NotLeaderForPartitionException.java 5adc72ccf2d0cf4c0503df8d19c945a2adb2df90 
>   clients/src/main/java/org/apache/kafka/common/errors/OffsetMetadataTooLarge.java a3159bb1034e784b5902d4b9f0e6fc863908899d 
>   clients/src/main/java/org/apache/kafka/common/errors/OffsetOutOfRangeException.java d01698a3efca7d4a9ee9b20f6f56abe2f299de06 
>   clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java ce95ca04aa842078ad20ea3ae2c764b51ac76f7a 
>   clients/src/main/java/org/apache/kafka/common/errors/RetriableException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/RetryableException.java c7f2f222f712a8b6659599c25fffc4fd55772d40 
>   clients/src/main/java/org/apache/kafka/common/errors/TimeoutException.java dffd64d19c35aa1f0378d220dedf619a9c115b6d 
>   clients/src/main/java/org/apache/kafka/common/errors/UnknownServerException.java a0690fe2870bff25cfc478ff0f3314a05a8af991 
>   clients/src/main/java/org/apache/kafka/common/errors/UnknownTopicOrPartitionException.java 73d1953cbe045d7f67e423ff2f0f4d27465db41d 
>   clients/src/main/java/org/apache/kafka/common/network/Selector.java 8ed4c73146b2e7d8bb0bb1779dd6f942b231a627 
>   clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java 1bbe83c1bfd7599dbba12424deee302e62c7a2b5 
>   clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java 41c028bffbda1c418d827a1f6a8d2a63cdf3e373 
> 
> Diff: https://reviews.apache.org/r/18199/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jay Kreps
> 
>

Re: Review Request 18199: Patch for KAFKA-1252

Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18199/
-----------------------------------------------------------

(Updated Feb. 19, 2014, 1:15 a.m.)


Review request for kafka.


Bugs: KAFKA-1252
    https://issues.apache.org/jira/browse/KAFKA-1252


Repository: kafka


Description
-------

KAFKA-1252 Implement retries in new producer.


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 3d180e885a25fa6b138f544ac320ec9e0d2a1e7f 
  clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java dca9802c8d0918cf5e284ff5b5ea2ccdf788602b 
  clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 52d30a86d04393ec06e8b362e91f31492a278680 
  clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java be8a4a399b8b43beabe2c34f8f4f728cb63a29bf 
  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/Cluster.java c17a8f8162db9386a36d4204b9036676344a3451 
  clients/src/main/java/org/apache/kafka/common/errors/CorruptRecordException.java 673f61d6271c55fe7f3422e49a3f1f8f7d3d2206 
  clients/src/main/java/org/apache/kafka/common/errors/InvalidMetadataException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/errors/LeaderNotAvailableException.java 0bde6b5a351fc2475181b927effdd5ee8c7ff85c 
  clients/src/main/java/org/apache/kafka/common/errors/NetworkException.java 3a041593d76cfb52471c49ca5a37ebf6129d8131 
  clients/src/main/java/org/apache/kafka/common/errors/NotLeaderForPartitionException.java 5adc72ccf2d0cf4c0503df8d19c945a2adb2df90 
  clients/src/main/java/org/apache/kafka/common/errors/OffsetMetadataTooLarge.java a3159bb1034e784b5902d4b9f0e6fc863908899d 
  clients/src/main/java/org/apache/kafka/common/errors/OffsetOutOfRangeException.java d01698a3efca7d4a9ee9b20f6f56abe2f299de06 
  clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java ce95ca04aa842078ad20ea3ae2c764b51ac76f7a 
  clients/src/main/java/org/apache/kafka/common/errors/RetriableException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/errors/RetryableException.java c7f2f222f712a8b6659599c25fffc4fd55772d40 
  clients/src/main/java/org/apache/kafka/common/errors/TimeoutException.java dffd64d19c35aa1f0378d220dedf619a9c115b6d 
  clients/src/main/java/org/apache/kafka/common/errors/UnknownServerException.java a0690fe2870bff25cfc478ff0f3314a05a8af991 
  clients/src/main/java/org/apache/kafka/common/errors/UnknownTopicOrPartitionException.java 73d1953cbe045d7f67e423ff2f0f4d27465db41d 
  clients/src/main/java/org/apache/kafka/common/network/Selector.java 8ed4c73146b2e7d8bb0bb1779dd6f942b231a627 
  clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java 1bbe83c1bfd7599dbba12424deee302e62c7a2b5 
  clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java 41c028bffbda1c418d827a1f6a8d2a63cdf3e373 

Diff: https://reviews.apache.org/r/18199/diff/


Testing
-------


Thanks,

Jay Kreps

Re: Review Request 18199: Patch for KAFKA-1252

Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18199/#review34705
-----------------------------------------------------------

Ship it!


+1. Just some minor comments below.


clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
<https://reviews.apache.org/r/18199/#comment64917>

    Should we call them sendBufferSize and receiveBufferSize?



clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java
<https://reviews.apache.org/r/18199/#comment65056>

    lastAttempt doesn't seem to be used.



clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
<https://reviews.apache.org/r/18199/#comment64925>

    If we want to be truly round robin, shouldn't nodeIndex to set to i+1?



clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
<https://reviews.apache.org/r/18199/#comment65058>

    Ditto as the above.



clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java
<https://reviews.apache.org/r/18199/#comment65061>

    Could we specify the parameter names here to make it clear?



clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java
<https://reviews.apache.org/r/18199/#comment65062>

    Could we specify the parameter names here to make it clear?


- Jun Rao


On Feb. 17, 2014, 11:25 p.m., Jay Kreps wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18199/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2014, 11:25 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1252
>     https://issues.apache.org/jira/browse/KAFKA-1252
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1252 Implement retries in new producer.
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 3d180e885a25fa6b138f544ac320ec9e0d2a1e7f 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java dca9802c8d0918cf5e284ff5b5ea2ccdf788602b 
>   clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 52d30a86d04393ec06e8b362e91f31492a278680 
>   clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java be8a4a399b8b43beabe2c34f8f4f728cb63a29bf 
>   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/Cluster.java c17a8f8162db9386a36d4204b9036676344a3451 
>   clients/src/main/java/org/apache/kafka/common/errors/CorruptRecordException.java 673f61d6271c55fe7f3422e49a3f1f8f7d3d2206 
>   clients/src/main/java/org/apache/kafka/common/errors/LeaderNotAvailableException.java 0bde6b5a351fc2475181b927effdd5ee8c7ff85c 
>   clients/src/main/java/org/apache/kafka/common/errors/NetworkException.java 3a041593d76cfb52471c49ca5a37ebf6129d8131 
>   clients/src/main/java/org/apache/kafka/common/errors/NotLeaderForPartitionException.java 5adc72ccf2d0cf4c0503df8d19c945a2adb2df90 
>   clients/src/main/java/org/apache/kafka/common/errors/OffsetOutOfRangeException.java d01698a3efca7d4a9ee9b20f6f56abe2f299de06 
>   clients/src/main/java/org/apache/kafka/common/errors/RetriableException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/RetryableException.java c7f2f222f712a8b6659599c25fffc4fd55772d40 
>   clients/src/main/java/org/apache/kafka/common/errors/TimeoutException.java dffd64d19c35aa1f0378d220dedf619a9c115b6d 
>   clients/src/main/java/org/apache/kafka/common/errors/UnknownTopicOrPartitionException.java 73d1953cbe045d7f67e423ff2f0f4d27465db41d 
>   clients/src/main/java/org/apache/kafka/common/network/Selector.java 8ed4c73146b2e7d8bb0bb1779dd6f942b231a627 
>   clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java 1bbe83c1bfd7599dbba12424deee302e62c7a2b5 
>   clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java 41c028bffbda1c418d827a1f6a8d2a63cdf3e373 
> 
> Diff: https://reviews.apache.org/r/18199/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jay Kreps
> 
>