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/07/19 00:45:29 UTC
Review Request 23697: Fix KAFKA-1533: Address Jun and Neha's comments
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23697/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1533
https://issues.apache.org/jira/browse/KAFKA-1533
Repository: kafka
Description
-------
1. Add the metadataRefreshAttemptMs in NetworkClient for backing off; 2. Refactor Producer API tests using KafkaTestHarness; 3. Change default backoff time to 100ms for test utils
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/NetworkClient.java d8f9ce663ee24d2b0852c974136741280c39f8f8
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 4aa5b01d611631db72df47d50bbe30edb8c478db
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 15fd5bcbaf175a0f7d7cf0b142e63f705ca9b6ae
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34a7db4b4ea2b720476c2b1f22a623a997faffbc
core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala 194dd70919a5f301d3131c56594e40a0ebb27311
core/src/test/scala/unit/kafka/utils/TestUtils.scala 3faa884f8eb83c7c00baab416d0acfb488dc39c1
Diff: https://reviews.apache.org/r/23697/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 23697: Fix KAFKA-1533: Address Jun and Neha's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
> On July 20, 2014, 11:32 p.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/clients/NetworkClient.java, lines 376-377
> > <https://reviews.apache.org/r/23697/diff/1/?file=635933#file635933line376>
> >
> > Thinking a bit more about this. There are two possibility after adding the metadata request to sends: (1) The metadata fetch succeeds. In this case Metadata.lastRefreshMs will be updated and this line is not needed. (2) The metadata fetch fails due to a connection issue. In this case, we probably want to fetch the metadata request from another node immediately, instead of backing off.
> >
> > So, it seems that in NetworkClient, we only need to take care of the case when no node is available for metadata. In this case, we should backoff. So, perhaps we should rename metadataLastUpdateAttemptMs to sth like lastNoNodeMs.
Actually for the second case, the metadata fetch can fail either the selected node is not connected any more or it cannot accept any more requests (i.e. inFlightRequests.canSendMore() == false). In the latter case, since this node is selected as least loaded, there will be no other connected nodes that is available for sending also, and hence we should also back off.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23697/#review48194
-----------------------------------------------------------
On July 18, 2014, 10:45 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23697/
> -----------------------------------------------------------
>
> (Updated July 18, 2014, 10:45 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1533
> https://issues.apache.org/jira/browse/KAFKA-1533
>
>
> Repository: kafka
>
>
> Description
> -------
>
> 1. Add the metadataRefreshAttemptMs in NetworkClient for backing off; 2. Refactor Producer API tests using KafkaTestHarness; 3. Change default backoff time to 100ms for test utils
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java d8f9ce663ee24d2b0852c974136741280c39f8f8
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 4aa5b01d611631db72df47d50bbe30edb8c478db
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 15fd5bcbaf175a0f7d7cf0b142e63f705ca9b6ae
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34a7db4b4ea2b720476c2b1f22a623a997faffbc
> core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala 194dd70919a5f301d3131c56594e40a0ebb27311
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 3faa884f8eb83c7c00baab416d0acfb488dc39c1
>
> Diff: https://reviews.apache.org/r/23697/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 23697: Fix KAFKA-1533: Address Jun and Neha's comments
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23697/#review48194
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/NetworkClient.java
<https://reviews.apache.org/r/23697/#comment84551>
For clarity, could we calculate the second part to a local val and give it a meaningful name?
clients/src/main/java/org/apache/kafka/clients/NetworkClient.java
<https://reviews.apache.org/r/23697/#comment84550>
Thinking a bit more about this. There are two possibility after adding the metadata request to sends: (1) The metadata fetch succeeds. In this case Metadata.lastRefreshMs will be updated and this line is not needed. (2) The metadata fetch fails due to a connection issue. In this case, we probably want to fetch the metadata request from another node immediately, instead of backing off.
So, it seems that in NetworkClient, we only need to take care of the case when no node is available for metadata. In this case, we should backoff. So, perhaps we should rename metadataLastUpdateAttemptMs to sth like lastNoNodeMs.
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/23697/#comment84552>
Indentation.
- Jun Rao
On July 18, 2014, 10:45 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23697/
> -----------------------------------------------------------
>
> (Updated July 18, 2014, 10:45 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1533
> https://issues.apache.org/jira/browse/KAFKA-1533
>
>
> Repository: kafka
>
>
> Description
> -------
>
> 1. Add the metadataRefreshAttemptMs in NetworkClient for backing off; 2. Refactor Producer API tests using KafkaTestHarness; 3. Change default backoff time to 100ms for test utils
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java d8f9ce663ee24d2b0852c974136741280c39f8f8
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 4aa5b01d611631db72df47d50bbe30edb8c478db
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 15fd5bcbaf175a0f7d7cf0b142e63f705ca9b6ae
> core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34a7db4b4ea2b720476c2b1f22a623a997faffbc
> core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala 194dd70919a5f301d3131c56594e40a0ebb27311
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 3faa884f8eb83c7c00baab416d0acfb488dc39c1
>
> Diff: https://reviews.apache.org/r/23697/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 23697: Fix KAFKA-1533: Address Jun and Neha's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23697/
-----------------------------------------------------------
(Updated July 21, 2014, 10:45 p.m.)
Review request for kafka.
Bugs: KAFKA-1533
https://issues.apache.org/jira/browse/KAFKA-1533
Repository: kafka
Description
-------
1. Add the metadataRefreshAttemptMs in NetworkClient for backing off; 2. Refactor Producer API tests using KafkaTestHarness; 3. Change default backoff time to 100ms for test utils
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/NetworkClient.java d8f9ce663ee24d2b0852c974136741280c39f8f8
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 4aa5b01d611631db72df47d50bbe30edb8c478db
core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 15fd5bcbaf175a0f7d7cf0b142e63f705ca9b6ae
core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 34a7db4b4ea2b720476c2b1f22a623a997faffbc
core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala 194dd70919a5f301d3131c56594e40a0ebb27311
core/src/test/scala/unit/kafka/utils/TestUtils.scala 3faa884f8eb83c7c00baab416d0acfb488dc39c1
Diff: https://reviews.apache.org/r/23697/diff/
Testing
-------
Thanks,
Guozhang Wang