You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jaikiran Pai <ja...@gmail.com> on 2015/01/09 04:59:43 UTC
Review Request 29752: Patch for KAFKA-1836
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29752/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1836
https://issues.apache.org/jira/browse/KAFKA-1836
Repository: kafka
Description
-------
KAFKA-1836 Don't block forever if metadata.fetch.timeout.ms is set to 0
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 1d30f9edd95337f86e632a09fc8f4126a67c238b
clients/src/test/java/org/apache/kafka/clients/producer/MetadataTest.java 4547bfcb44be4d72742076d6e93f424b3b22a7a9
Diff: https://reviews.apache.org/r/29752/diff/
Testing
-------
Thanks,
Jaikiran Pai
Re: Review Request 29752: Patch for KAFKA-1836
Posted by Jaikiran Pai <ja...@gmail.com>.
> On Jan. 9, 2015, 5:51 p.m., Ewen Cheslack-Postava wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java, line 110
> > <https://reviews.apache.org/r/29752/diff/1/?file=813990#file813990line110>
> >
> > This works since the check farther down ensures remainingWaitMs is never 0 unless maxWaitMs is 0, but it's a bit confusing when reading this code why we check maxWaitMs instead of remainingWaitMs. There's no functional difference, but I think checking remainingWaitMs instead would be clearer for someone reading this code.
Updated to change this and check the remainingWaitMs.
Thanks Ewen.
- Jaikiran
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29752/#review67457
-----------------------------------------------------------
On Jan. 10, 2015, 2:57 a.m., Jaikiran Pai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29752/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2015, 2:57 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1836
> https://issues.apache.org/jira/browse/KAFKA-1836
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1836 Don't block forever if metadata.fetch.timeout.ms is set to 0
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 1d30f9edd95337f86e632a09fc8f4126a67c238b
> clients/src/test/java/org/apache/kafka/clients/producer/MetadataTest.java 4547bfcb44be4d72742076d6e93f424b3b22a7a9
>
> Diff: https://reviews.apache.org/r/29752/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jaikiran Pai
>
>
Re: Review Request 29752: Patch for KAFKA-1836
Posted by Ewen Cheslack-Postava <me...@ewencp.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29752/#review67457
-----------------------------------------------------------
Ship it!
One minor nit, but looks good to me. Non-binding +1.
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java
<https://reviews.apache.org/r/29752/#comment111484>
This works since the check farther down ensures remainingWaitMs is never 0 unless maxWaitMs is 0, but it's a bit confusing when reading this code why we check maxWaitMs instead of remainingWaitMs. There's no functional difference, but I think checking remainingWaitMs instead would be clearer for someone reading this code.
- Ewen Cheslack-Postava
On Jan. 9, 2015, 3:59 a.m., Jaikiran Pai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29752/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2015, 3:59 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1836
> https://issues.apache.org/jira/browse/KAFKA-1836
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1836 Don't block forever if metadata.fetch.timeout.ms is set to 0
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 1d30f9edd95337f86e632a09fc8f4126a67c238b
> clients/src/test/java/org/apache/kafka/clients/producer/MetadataTest.java 4547bfcb44be4d72742076d6e93f424b3b22a7a9
>
> Diff: https://reviews.apache.org/r/29752/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jaikiran Pai
>
>
Re: Review Request 29752: Patch for KAFKA-1836
Posted by Jaikiran Pai <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29752/
-----------------------------------------------------------
(Updated Jan. 10, 2015, 2:57 a.m.)
Review request for kafka.
Bugs: KAFKA-1836
https://issues.apache.org/jira/browse/KAFKA-1836
Repository: kafka
Description
-------
KAFKA-1836 Don't block forever if metadata.fetch.timeout.ms is set to 0
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 1d30f9edd95337f86e632a09fc8f4126a67c238b
clients/src/test/java/org/apache/kafka/clients/producer/MetadataTest.java 4547bfcb44be4d72742076d6e93f424b3b22a7a9
Diff: https://reviews.apache.org/r/29752/diff/
Testing
-------
Thanks,
Jaikiran Pai
Re: Review Request 29752: Patch for KAFKA-1836
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29752/#review67466
-----------------------------------------------------------
Ship it!
Looks good other than what Ewen pointed out.
- Neha Narkhede
On Jan. 9, 2015, 3:59 a.m., Jaikiran Pai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29752/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2015, 3:59 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1836
> https://issues.apache.org/jira/browse/KAFKA-1836
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1836 Don't block forever if metadata.fetch.timeout.ms is set to 0
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 1d30f9edd95337f86e632a09fc8f4126a67c238b
> clients/src/test/java/org/apache/kafka/clients/producer/MetadataTest.java 4547bfcb44be4d72742076d6e93f424b3b22a7a9
>
> Diff: https://reviews.apache.org/r/29752/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jaikiran Pai
>
>