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 2020/05/11 08:53:34 UTC
[GitHub] [kafka] showuon opened a new pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
showuon opened a new pull request #8646:
URL: https://github.com/apache/kafka/pull/8646
> java.lang.AssertionError: Expected: is <0L> but: was <43L> at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20) at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6) at org.apache.kafka.streams.integration.OptimizedKTableIntegrationTest.shouldApplyUpdatesToStandbyStore(OptimizedKTableIntegrationTest.java:138)
The tests failed at `assertThat(listener.startOffset, is(equalTo(0L)));`. It looks like that it did a restore before the assert. But we should expect the restore sometimes happen to resume the failed tasks by itself. It should not cause the test failure under this situation.
On the other hands, I checked the original tests in below PR link:
https://github.com/apache/kafka/pull/7238/files#diff-7b659da73450d5bf7a1731b5606e4c83R205
The original tests added the `assertThat(listener.startOffset, is(equalTo(0L)));` is because in the end of the test, we'll also test the `startOffset` value. But in the newer version of the test, we don't really care about the `startOffset` or `totalNumRestored` value. All we want to test in this test is:
**Assert that the current value in store reflects all messages being processed**
So, removing the assert can avoid flaky test failure, and also be able to test what the test case want to test.
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#discussion_r428519103
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/OptimizedKTableIntegrationTest.java
##########
@@ -133,11 +133,6 @@ public void shouldApplyUpdatesToStandbyStore() throws Exception {
kafkaStreams1WasFirstActive = false;
}
- // Assert that no restore has occurred, ensures that when we check later that the restore
- // notification actually came from after the rebalance.
- assertThat(listener.startOffset, is(equalTo(0L)));
- assertThat(listener.totalNumRestored, is(equalTo(0L)));
Review comment:
Sorry @mjsax , I tried to trace the code to find out why that would happen, but I still can't figure it out. Do you have any suggestion for it? If not, I think we can have more clear message output when this assert failure happen again. I'm not sure if the info is enough if the error happened again. And not sure if you have any thoughts about it?
It'll look like this when assert failure.
Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#issuecomment-626567795
Hi @mjsax , could you please check this PR to fix a flaky test when available? Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#issuecomment-644030596
Thanks for the suggestion, @guozhangwang , I removed the listeners. I agree we can fix it in this way first, and keep monitoring it. Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#issuecomment-644902264
Cherry-picked to 2.6.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#issuecomment-644547455
test this
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#discussion_r438359000
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/OptimizedKTableIntegrationTest.java
##########
@@ -133,11 +133,6 @@ public void shouldApplyUpdatesToStandbyStore() throws Exception {
kafkaStreams1WasFirstActive = false;
}
- // Assert that no restore has occurred, ensures that when we check later that the restore
- // notification actually came from after the rebalance.
- assertThat(listener.startOffset, is(equalTo(0L)));
- assertThat(listener.totalNumRestored, is(equalTo(0L)));
Review comment:
I think one possible explanation is that when we start the two instances, the rebalances took not just once but twice (once for the first instance, and another time for the second). In between the task may already be processed a bit on the first instance, and then after the second rebalance it was migrated and hence was restored a bit causing the listener to be triggered.
I'd agree with @showuon here that we do not need to strictly forbids restoration not happening when starting the first two instances, just making sure after closing one instance we can restore the other up to the first batch is enough (which is already validated). So I'm fine with removing this listener all together.
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/OptimizedKTableIntegrationTest.java
##########
@@ -133,11 +133,6 @@ public void shouldApplyUpdatesToStandbyStore() throws Exception {
kafkaStreams1WasFirstActive = false;
}
- // Assert that no restore has occurred, ensures that when we check later that the restore
- // notification actually came from after the rebalance.
- assertThat(listener.startOffset, is(equalTo(0L)));
- assertThat(listener.totalNumRestored, is(equalTo(0L)));
Review comment:
BTW there's another report that line 159 can also fail:
```
java.lang.AssertionError: Expected: is <true> but: was <false> at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20) at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6) at org.apache.kafka.streams.integration.OptimizedKTableIntegrationTest.shouldApplyUpdatesToStandbyStore(OptimizedKTableIntegrationTest.java:159)
```
Which is a bit mystery to me, since I cannot really think of a way how that could happen.
ANyways, for now removing that listener all together seems good to me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#discussion_r429501759
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/OptimizedKTableIntegrationTest.java
##########
@@ -133,11 +133,6 @@ public void shouldApplyUpdatesToStandbyStore() throws Exception {
kafkaStreams1WasFirstActive = false;
}
- // Assert that no restore has occurred, ensures that when we check later that the restore
- // notification actually came from after the rebalance.
- assertThat(listener.startOffset, is(equalTo(0L)));
- assertThat(listener.totalNumRestored, is(equalTo(0L)));
Review comment:
I am not 100% sure about the root cause of the issue... If you have a good suggestion for a better error message, that would be great. I have not idea how we could improve it atm.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#issuecomment-644247677
test this
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang merged pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
guozhangwang merged pull request #8646:
URL: https://github.com/apache/kafka/pull/8646
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#issuecomment-644547384
test this
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#issuecomment-644543006
**JDK 8 and Scala 2.12**
failed kafka.admin.ReassignPartitionsUnitTest.testModifyBrokerThrottles
--> traced in KAFKA-10155, PR: https://github.com/apache/kafka/pull/8853
**JDK 14 and Scala 2.13**
failed org.apache.kafka.streams.integration.HighAvailabilityTaskAssignorIntegrationTest.shouldScaleOutWithWarmupTasksAndPersistentStores
--> passed locally
kafka.admin.ReassignPartitionsUnitTest.testModifyBrokerThrottles
--> traced in KAFKA-10155, PR: https://github.com/apache/kafka/pull/8853
**JDK 11 and Scala 2.13**
org.apache.kafka.connect.mirror.MirrorConnectorsIntegrationTest.testReplication
--> passed locally
kafka.api.PlaintextConsumerTest.testLowMaxFetchSizeForRequestAndPartition
kafka.api.PlaintextConsumerTest.testLowMaxFetchSizeForRequestAndPartition
--> traced in KAFKA-8460.
kafka.admin.ReassignPartitionsUnitTest.testModifyBrokerThrottles
kafka.admin.ReassignPartitionsUnitTest.testModifyBrokerThrottles
--> traced in KAFKA-10155, PR: https://github.com/apache/kafka/pull/8853
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon edited a comment on pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#issuecomment-644030596
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon edited a comment on pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#issuecomment-644543006
**JDK 8 and Scala 2.12**
failed kafka.admin.ReassignPartitionsUnitTest.testModifyBrokerThrottles
--> traced in KAFKA-10155, PR: https://github.com/apache/kafka/pull/8853
**JDK 14 and Scala 2.13**
failed org.apache.kafka.streams.integration.HighAvailabilityTaskAssignorIntegrationTest.shouldScaleOutWithWarmupTasksAndPersistentStores
--> passed locally
kafka.admin.ReassignPartitionsUnitTest.testModifyBrokerThrottles
--> traced in KAFKA-10155, PR: https://github.com/apache/kafka/pull/8853
**JDK 11 and Scala 2.13**
org.apache.kafka.connect.mirror.MirrorConnectorsIntegrationTest.testReplication
--> passed locally
kafka.api.PlaintextConsumerTest.testLowMaxFetchSizeForRequestAndPartition
kafka.api.PlaintextConsumerTest.testLowMaxFetchSizeForRequestAndPartition
--> traced in KAFKA-8460 KAFKA-8264
kafka.admin.ReassignPartitionsUnitTest.testModifyBrokerThrottles
kafka.admin.ReassignPartitionsUnitTest.testModifyBrokerThrottles
--> traced in KAFKA-10155, PR: https://github.com/apache/kafka/pull/8853
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #8646: KAFKA-9974: Fix flaky test by removing unneeded asserts
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #8646:
URL: https://github.com/apache/kafka/pull/8646#discussion_r428519103
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/OptimizedKTableIntegrationTest.java
##########
@@ -133,11 +133,6 @@ public void shouldApplyUpdatesToStandbyStore() throws Exception {
kafkaStreams1WasFirstActive = false;
}
- // Assert that no restore has occurred, ensures that when we check later that the restore
- // notification actually came from after the rebalance.
- assertThat(listener.startOffset, is(equalTo(0L)));
- assertThat(listener.totalNumRestored, is(equalTo(0L)));
Review comment:
Sorry @mjsax , I tried to trace the code to find out why that would happen, but I still can't figure it out. Do you have any suggestion for it? If not, I think we can at least have more clear message output when this assert failure happen again. I'm not sure if the info is enough if the error happened again. And not sure if you have any thoughts about it?
It'll look like this when assert failure.
Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org