You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Ashish Singh <as...@cloudera.com> on 2015/06/30 02:46:15 UTC
Review Request 36030: Patch for KAFKA-972
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-972
https://issues.apache.org/jira/browse/KAFKA-972
Repository: kafka
Description
-------
KAFKA-972: MetadataRequest returns stale list of brokers
Diffs
-----
core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
Diff: https://reviews.apache.org/r/36030/diff/
Testing
-------
Thanks,
Ashish Singh
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
> On July 1, 2015, 4:37 a.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala, line 69
> > <https://reviews.apache.org/r/36030/diff/2/?file=996282#file996282line69>
> >
> > Do we need this? In tearDown(), ZookeeperTestHarness will delete all ZK data.
Ahh.. I thought its called after all tests in the class are done. Thanks for pointing this out. Removed.
> On July 1, 2015, 4:37 a.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala, lines 157-161
> > <https://reviews.apache.org/r/36030/diff/2/?file=996282#file996282line157>
> >
> > Could we issue TopicMetadataRequest to every broker and make sure that the correct metadata is propagated to every broker? Ditto in other places as well.
Done!
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/#review90005
-----------------------------------------------------------
On July 1, 2015, 8:37 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36030/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 8:37 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-972
> https://issues.apache.org/jira/browse/KAFKA-972
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-972: MetadataRequest returns stale list of brokers
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
> core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 995b05901491bb0dbf0df210d44bd1d7f66fdc82
>
> Diff: https://reviews.apache.org/r/36030/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36030: Patch for KAFKA-972
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/#review90005
-----------------------------------------------------------
Thanks for the patch. A couple more comments.
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala (line 69)
<https://reviews.apache.org/r/36030/#comment142931>
Do we need this? In tearDown(), ZookeeperTestHarness will delete all ZK data.
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala (lines 157 - 161)
<https://reviews.apache.org/r/36030/#comment142932>
Could we issue TopicMetadataRequest to every broker and make sure that the correct metadata is propagated to every broker? Ditto in other places as well.
- Jun Rao
On July 1, 2015, 1:42 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36030/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 1:42 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-972
> https://issues.apache.org/jira/browse/KAFKA-972
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-972: MetadataRequest returns stale list of brokers
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
> core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 995b05901491bb0dbf0df210d44bd1d7f66fdc82
>
> Diff: https://reviews.apache.org/r/36030/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/
-----------------------------------------------------------
(Updated July 8, 2015, 6:24 a.m.)
Review request for kafka.
Bugs: KAFKA-972
https://issues.apache.org/jira/browse/KAFKA-972
Repository: kafka
Description
-------
KAFKA-972: MetadataRequest returns stale list of brokers
Diffs (updated)
-----
core/src/main/scala/kafka/controller/KafkaController.scala 20f1499046c768adbcd2bf8ad5969589c8641f34
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala a95ee5e0849d29a5f95fdabed4f1988a308e9872
Diff: https://reviews.apache.org/r/36030/diff/
Testing
-------
Thanks,
Ashish Singh
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
> On July 8, 2015, 12:52 a.m., Jun Rao wrote:
> > Thanks for the patch. Saw the following transient unit test failure.
> >
> > kafka.integration.TopicMetadataTest > testIsrAfterBrokerShutDownAndJoinsBack FAILED
> > junit.framework.AssertionFailedError: Topic metadata is not correctly updated for broker kafka.server.KafkaServer@5df78dd0.
> > Expected ISR: List(BrokerEndPoint(0,localhost,59755), BrokerEndPoint(1,localhost,59758), BrokerEndPoint(2,localhost,59762), BrokerEndPoint(3,localhost,59789))
> > Actual ISR :
> > at junit.framework.Assert.fail(Assert.java:47)
> > at kafka.utils.TestUtils$.waitUntilTrue(TestUtils.scala:619)
> > at kafka.integration.TopicMetadataTest$$anonfun$checkIsr$1.apply(TopicMetadataTest.scala:149)
> > at kafka.integration.TopicMetadataTest$$anonfun$checkIsr$1.apply(TopicMetadataTest.scala:147)
> > at scala.collection.immutable.List.foreach(List.scala:318)
> > at kafka.integration.TopicMetadataTest.checkIsr(TopicMetadataTest.scala:147)
> > at kafka.integration.TopicMetadataTest.testIsrAfterBrokerShutDownAndJoinsBack(TopicMetadataTest.scala:190)
I tried it a few times before submitting the patch and it passed. Wait time for ISR to get updated on all brokers running on same machine will probably be proportional to number of brokers, so I made wait time 5000L * numConfigs. However, it is not necessary that ISRs will be updated in that time. I think we have two options, first option is to increase the wait time, second option is to limit the number of brokers to 2 in the test case. I prefer second option, as the test case only requires 2 brokers for the scenario it is testing and will avoid making the test take longer time than an unit should be taking. I ran the test 20 times back to back with second approach and it works fine. Let me know if it still does not work for you.
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/#review90843
-----------------------------------------------------------
On July 7, 2015, 5:42 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36030/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 5:42 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-972
> https://issues.apache.org/jira/browse/KAFKA-972
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-972: MetadataRequest returns stale list of brokers
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 09630d07afc75d4b92c847e31032f8a1dfa0dabe
> core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala a95ee5e0849d29a5f95fdabed4f1988a308e9872
>
> Diff: https://reviews.apache.org/r/36030/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36030: Patch for KAFKA-972
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/#review90843
-----------------------------------------------------------
Thanks for the patch. Saw the following transient unit test failure.
kafka.integration.TopicMetadataTest > testIsrAfterBrokerShutDownAndJoinsBack FAILED
junit.framework.AssertionFailedError: Topic metadata is not correctly updated for broker kafka.server.KafkaServer@5df78dd0.
Expected ISR: List(BrokerEndPoint(0,localhost,59755), BrokerEndPoint(1,localhost,59758), BrokerEndPoint(2,localhost,59762), BrokerEndPoint(3,localhost,59789))
Actual ISR :
at junit.framework.Assert.fail(Assert.java:47)
at kafka.utils.TestUtils$.waitUntilTrue(TestUtils.scala:619)
at kafka.integration.TopicMetadataTest$$anonfun$checkIsr$1.apply(TopicMetadataTest.scala:149)
at kafka.integration.TopicMetadataTest$$anonfun$checkIsr$1.apply(TopicMetadataTest.scala:147)
at scala.collection.immutable.List.foreach(List.scala:318)
at kafka.integration.TopicMetadataTest.checkIsr(TopicMetadataTest.scala:147)
at kafka.integration.TopicMetadataTest.testIsrAfterBrokerShutDownAndJoinsBack(TopicMetadataTest.scala:190)
- Jun Rao
On July 7, 2015, 5:42 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36030/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 5:42 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-972
> https://issues.apache.org/jira/browse/KAFKA-972
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-972: MetadataRequest returns stale list of brokers
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 09630d07afc75d4b92c847e31032f8a1dfa0dabe
> core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala a95ee5e0849d29a5f95fdabed4f1988a308e9872
>
> Diff: https://reviews.apache.org/r/36030/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/
-----------------------------------------------------------
(Updated July 7, 2015, 5:42 p.m.)
Review request for kafka.
Bugs: KAFKA-972
https://issues.apache.org/jira/browse/KAFKA-972
Repository: kafka
Description
-------
KAFKA-972: MetadataRequest returns stale list of brokers
Diffs (updated)
-----
core/src/main/scala/kafka/controller/KafkaController.scala 09630d07afc75d4b92c847e31032f8a1dfa0dabe
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala a95ee5e0849d29a5f95fdabed4f1988a308e9872
Diff: https://reviews.apache.org/r/36030/diff/
Testing
-------
Thanks,
Ashish Singh
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
> On July 7, 2015, 4:50 p.m., Jun Rao wrote:
> > Thanks for the latest patch. Looks good. Could you rebase?
Done.
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/#review90721
-----------------------------------------------------------
On July 7, 2015, 5:42 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36030/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 5:42 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-972
> https://issues.apache.org/jira/browse/KAFKA-972
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-972: MetadataRequest returns stale list of brokers
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 09630d07afc75d4b92c847e31032f8a1dfa0dabe
> core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala a95ee5e0849d29a5f95fdabed4f1988a308e9872
>
> Diff: https://reviews.apache.org/r/36030/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36030: Patch for KAFKA-972
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/#review90721
-----------------------------------------------------------
Thanks for the latest patch. Looks good. Could you rebase?
- Jun Rao
On July 7, 2015, 6:07 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36030/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 6:07 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-972
> https://issues.apache.org/jira/browse/KAFKA-972
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-972: MetadataRequest returns stale list of brokers
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
> core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 995b05901491bb0dbf0df210d44bd1d7f66fdc82
>
> Diff: https://reviews.apache.org/r/36030/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/
-----------------------------------------------------------
(Updated July 7, 2015, 6:07 a.m.)
Review request for kafka.
Bugs: KAFKA-972
https://issues.apache.org/jira/browse/KAFKA-972
Repository: kafka
Description
-------
KAFKA-972: MetadataRequest returns stale list of brokers
Diffs (updated)
-----
core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 995b05901491bb0dbf0df210d44bd1d7f66fdc82
Diff: https://reviews.apache.org/r/36030/diff/
Testing
-------
Thanks,
Ashish Singh
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
> On July 6, 2015, 9:47 p.m., Jun Rao wrote:
> > Thanks for the patch. A few more minor comments blow.
Thanks for the review again Jun!
> On July 6, 2015, 9:47 p.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala, line 147
> > <https://reviews.apache.org/r/36030/diff/5/?file=996624#file996624line147>
> >
> > This can be private, right?
It should be. Thanks for pointing out.
> On July 6, 2015, 9:47 p.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala, lines 148-156
> > <https://reviews.apache.org/r/36030/diff/5/?file=996624#file996624line148>
> >
> > This seems redundant given the code in 155 to 163. We can probaby just assert the broker size on topicMetadata after line 152.
True
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/#review90579
-----------------------------------------------------------
On July 7, 2015, 6:07 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36030/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 6:07 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-972
> https://issues.apache.org/jira/browse/KAFKA-972
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-972: MetadataRequest returns stale list of brokers
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
> core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 995b05901491bb0dbf0df210d44bd1d7f66fdc82
>
> Diff: https://reviews.apache.org/r/36030/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36030: Patch for KAFKA-972
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/#review90579
-----------------------------------------------------------
Thanks for the patch. A few more minor comments blow.
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala (line 139)
<https://reviews.apache.org/r/36030/#comment143678>
This can be private, right?
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala (lines 140 - 148)
<https://reviews.apache.org/r/36030/#comment143681>
This seems redundant given the code in 155 to 163. We can probaby just assert the broker size on topicMetadata after line 152.
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala (line 155)
<https://reviews.apache.org/r/36030/#comment143682>
It's better to use foreach() instead map() in this case since we don't need the output of map.
- Jun Rao
On July 1, 2015, 3:06 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36030/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 3:06 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-972
> https://issues.apache.org/jira/browse/KAFKA-972
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-972: MetadataRequest returns stale list of brokers
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
> core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 995b05901491bb0dbf0df210d44bd1d7f66fdc82
>
> Diff: https://reviews.apache.org/r/36030/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/
-----------------------------------------------------------
(Updated July 1, 2015, 3:06 p.m.)
Review request for kafka.
Bugs: KAFKA-972
https://issues.apache.org/jira/browse/KAFKA-972
Repository: kafka
Description
-------
KAFKA-972: MetadataRequest returns stale list of brokers
Diffs (updated)
-----
core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 995b05901491bb0dbf0df210d44bd1d7f66fdc82
Diff: https://reviews.apache.org/r/36030/diff/
Testing
-------
Thanks,
Ashish Singh
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
> On July 1, 2015, 2:18 p.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala, lines 162-172
> > <https://reviews.apache.org/r/36030/diff/4/?file=996501#file996501line162>
> >
> > The propagation of the metadata to different brokers are independant. So,we will need to wrap the test on each broker with waitUntilTrue.
True, made the change.
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/#review90045
-----------------------------------------------------------
On July 1, 2015, 3:06 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36030/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 3:06 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-972
> https://issues.apache.org/jira/browse/KAFKA-972
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-972: MetadataRequest returns stale list of brokers
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
> core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 995b05901491bb0dbf0df210d44bd1d7f66fdc82
>
> Diff: https://reviews.apache.org/r/36030/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36030: Patch for KAFKA-972
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/#review90045
-----------------------------------------------------------
Thanks for the patch. Just one more comment.
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala (lines 154 - 164)
<https://reviews.apache.org/r/36030/#comment142985>
The propagation of the metadata to different brokers are independant. So,we will need to wrap the test on each broker with waitUntilTrue.
- Jun Rao
On July 1, 2015, 8:43 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36030/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 8:43 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-972
> https://issues.apache.org/jira/browse/KAFKA-972
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-972: MetadataRequest returns stale list of brokers
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
> core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 995b05901491bb0dbf0df210d44bd1d7f66fdc82
>
> Diff: https://reviews.apache.org/r/36030/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/
-----------------------------------------------------------
(Updated July 1, 2015, 8:43 a.m.)
Review request for kafka.
Bugs: KAFKA-972
https://issues.apache.org/jira/browse/KAFKA-972
Repository: kafka
Description
-------
KAFKA-972: MetadataRequest returns stale list of brokers
Diffs (updated)
-----
core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 995b05901491bb0dbf0df210d44bd1d7f66fdc82
Diff: https://reviews.apache.org/r/36030/diff/
Testing
-------
Thanks,
Ashish Singh
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/
-----------------------------------------------------------
(Updated July 1, 2015, 8:37 a.m.)
Review request for kafka.
Bugs: KAFKA-972
https://issues.apache.org/jira/browse/KAFKA-972
Repository: kafka
Description
-------
KAFKA-972: MetadataRequest returns stale list of brokers
Diffs (updated)
-----
core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 995b05901491bb0dbf0df210d44bd1d7f66fdc82
Diff: https://reviews.apache.org/r/36030/diff/
Testing
-------
Thanks,
Ashish Singh
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/
-----------------------------------------------------------
(Updated July 1, 2015, 1:42 a.m.)
Review request for kafka.
Bugs: KAFKA-972
https://issues.apache.org/jira/browse/KAFKA-972
Repository: kafka
Description
-------
KAFKA-972: MetadataRequest returns stale list of brokers
Diffs (updated)
-----
core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 995b05901491bb0dbf0df210d44bd1d7f66fdc82
Diff: https://reviews.apache.org/r/36030/diff/
Testing
-------
Thanks,
Ashish Singh
Re: Review Request 36030: Patch for KAFKA-972
Posted by Ashish Singh <as...@cloudera.com>.
> On June 30, 2015, 4:14 p.m., Jun Rao wrote:
> > Thanks for the patch. A few comments below.
> >
> > 1. Could you add a unit test for this?
> >
> > 2. Also, we may have a similar issue when handling the failure of the broker. Currently, the logic in the controller is that the controller will only send an UpdateMetadataRequest to all brokers if a new leader is elected. So, when shutting down a broker with no leaders on it, the controller won't send UpdateMetadataRequest to reflect the shutdown broker. Could you verify if this is indeed an issue?
Thanks for the review Jun. Added tests. Yes, (2) is an issue, added fix with test to catch it.
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/#review89907
-----------------------------------------------------------
On June 30, 2015, 12:46 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36030/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 12:46 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-972
> https://issues.apache.org/jira/browse/KAFKA-972
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-972: MetadataRequest returns stale list of brokers
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
>
> Diff: https://reviews.apache.org/r/36030/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 36030: Patch for KAFKA-972
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36030/#review89907
-----------------------------------------------------------
Thanks for the patch. A few comments below.
1. Could you add a unit test for this?
2. Also, we may have a similar issue when handling the failure of the broker. Currently, the logic in the controller is that the controller will only send an UpdateMetadataRequest to all brokers if a new leader is elected. So, when shutting down a broker with no leaders on it, the controller won't send UpdateMetadataRequest to reflect the shutdown broker. Could you verify if this is indeed an issue?
core/src/main/scala/kafka/controller/KafkaController.scala (lines 399 - 401)
<https://reviews.apache.org/r/36030/#comment142784>
Could you change the comments accordingly?
core/src/main/scala/kafka/controller/KafkaController.scala (line 402)
<https://reviews.apache.org/r/36030/#comment142783>
We probably should use liveOrShuttingDownBrokerIds.
- Jun Rao
On June 30, 2015, 12:46 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36030/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 12:46 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-972
> https://issues.apache.org/jira/browse/KAFKA-972
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-972: MetadataRequest returns stale list of brokers
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/controller/KafkaController.scala 36350579b16027359d237b64699003358704ac6f
>
> Diff: https://reviews.apache.org/r/36030/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ashish Singh
>
>