-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-5063: Flaky ResetIntegrationTest #2931
Conversation
Call for review @enothereska @dguy @guozhangwang |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
I noticed one of the wait functions does not have the 60000 timeout. |
@@ -187,7 +187,8 @@ public void testReprocessingFromScratchAfterResetWithIntermediateUserTopic() thr | |||
streams.close(); | |||
|
|||
assertThat(resultRerun, equalTo(result)); | |||
assertThat(resultRerun2, equalTo(result2)); | |||
final int maxToCompare = Math.max(result2.size(), resultRerun2.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure if this should be max
or min
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be min -- good catch! (problem: the test passed for me locally all the time, so this "fix" does not change local runs)
Also there is a test that failed, but it seems the results are not there anymore. |
retest this please |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
About the timeout -- I guess we can got back to default anyway -- increasing the timeouts to 60000 was a try to stabilize the test -- it was not a proper fix though. WDTY about this? |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
@@ -187,7 +185,8 @@ public void testReprocessingFromScratchAfterResetWithIntermediateUserTopic() thr | |||
streams.close(); | |||
|
|||
assertThat(resultRerun, equalTo(result)); | |||
assertThat(resultRerun2, equalTo(result2)); | |||
final int maxToCompare = Math.min(result2.size(), resultRerun2.size()); | |||
assertThat(resultRerun2.subList(0, maxToCompare), equalTo(result2.subList(0, maxToCompare))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the results not identical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is somewhat non-deterministic with respect to the expected result. Because, CLUSTER
mock time get's initialized with System.currentTimeMillis
, the timestamps assigned to the input topic records vary, and thus, they can fall into different windows in each run. And therefore, we don't know exactly how many records we need to expect. For this reason, we only wait for the first 10 records -- however, IntegrationTestUtils.waitUntilMinKeyValueRecordsReceived
does not return exactly 10 records, but at-least 10 records, because the actual fetch size of the consumer can vary -- thus, we can end up with a different number of returned record for both runs (even if the topic contains the same data). However, the common prefix must be the same and thus we use it for the comparison.
An alternative (more complex) fix would be, to use the initial mock timestamp to figure out the windows in which records fall into, and thus compute the number the expected result record exactly. But I don't think this would make the test better, but only complicates the code.
Does this make sense? \cc @enothereska @guozhangwang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjsax With the recent changes on IntegrationTestUtils, waitUntilXXX
will use one consumer instance only so I'm wondering if it could still return more than expected results.
Also for the mock time issue, could we eliminate the non-determinism by not using System.currentTimeMillis
? We can augment the MockTime
to set manual initialization values if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing MockTime
would fix the issue by itself. Will try to do that.
Is this still needed? |
retest this please |
I think yes -- the failure we observed is unrelated to the consumer reset strategy (cf https://github.com/apache/kafka/pull/2931/files#r114402394). The prefix did always match -- the test failed because different number of records got returned and thus the lists did not match because of different size. |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Updated this. |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Retest this please. |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Retest this please |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
retest this please |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Created https://issues.apache.org/jira/browse/KAFKA-5209 for the newly observed transient failures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged to trunk.
No description provided.