You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/05/20 10:33:13 UTC
[GitHub] [kafka] divijvaidya opened a new pull request, #12189: Fix flaky test TopicCommandIntegrationTest.testDescribeAtMinIsrPartitions(String).quorum=kraft
divijvaidya opened a new pull request, #12189:
URL: https://github.com/apache/kafka/pull/12189
## Problem
Flaky test as failed in CI https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-12184/1/tests/
The test fails because it does not wait for metadata to be propagated across brokers before killing a broker which may lead to it getting stale information. Note that a similar test was done in #12104 for a different test.
## Change
Wait for metadata propagation to complete after killing the broker.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a diff in pull request #12189: Fix flaky test TopicCommandIntegrationTest.testDescribeAtMinIsrPartitions(String).quorum=kraft
Posted by GitBox <gi...@apache.org>.
hachikuji commented on code in PR #12189:
URL: https://github.com/apache/kafka/pull/12189#discussion_r878330637
##########
core/src/test/scala/integration/kafka/admin/TopicCommandIntegrationTest.scala:
##########
@@ -697,6 +697,19 @@ class TopicCommandIntegrationTest extends KafkaServerTestHarness with Logging wi
try {
killBroker(0)
killBroker(1)
+
+ val aliveServers = brokers.filterNot(brokers => brokers.config.brokerId == 0 || brokers.config.brokerId == 1)
+
+ if (isKRaftTest()) {
+ TestUtils.ensureConsistentKRaftMetadata(aliveServers, controllerServer,
Review Comment:
We have a helper `KafkaServerTestHarness.ensureConsistentKRaftMetadata`. I think it would make sense to modify it to use only the alive brokers. Then we could use it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang merged pull request #12189: Fix flaky test TopicCommandIntegrationTest.testDescribeAtMinIsrPartitions(String).quorum=kraft
Posted by GitBox <gi...@apache.org>.
guozhangwang merged PR #12189:
URL: https://github.com/apache/kafka/pull/12189
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12189: Fix flaky test TopicCommandIntegrationTest.testDescribeAtMinIsrPartitions(String).quorum=kraft
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12189:
URL: https://github.com/apache/kafka/pull/12189#issuecomment-1132747105
@dengziming @guozhangwang perhaps you might be interested to code review this one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12189: Fix flaky test TopicCommandIntegrationTest.testDescribeAtMinIsrPartitions(String).quorum=kraft
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12189:
URL: https://github.com/apache/kafka/pull/12189#discussion_r878437206
##########
core/src/test/scala/integration/kafka/admin/TopicCommandIntegrationTest.scala:
##########
@@ -697,6 +697,19 @@ class TopicCommandIntegrationTest extends KafkaServerTestHarness with Logging wi
try {
killBroker(0)
killBroker(1)
+
+ val aliveServers = brokers.filterNot(brokers => brokers.config.brokerId == 0 || brokers.config.brokerId == 1)
+
+ if (isKRaftTest()) {
+ TestUtils.ensureConsistentKRaftMetadata(aliveServers, controllerServer,
Review Comment:
Thank you for the review and suggestion. I have made two changes based on your suggestion:
1. Added a helper method `aliveBrokers` in KafkaServerTestHarness()
2. Modified KafkaServerTestHarness.ensureConsistentKRaftMetadata() to check for only the brokers which are alive
Separately, if you have thoughts on further simplification (especially around combining the logic of Zk and kraft in a single helper function), I would be happy to make the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org