Skip to content

KAFKA-8026: Fix for flaky RegexSourceIntegrationTest#6459

Merged
bbejeck merged 3 commits intoapache:1.1from
bbejeck:KAFKA-8026_fix_flaky_regex_source_integration_test
Mar 25, 2019
Merged

KAFKA-8026: Fix for flaky RegexSourceIntegrationTest#6459
bbejeck merged 3 commits intoapache:1.1from
bbejeck:KAFKA-8026_fix_flaky_regex_source_integration_test

Conversation

@bbejeck
Copy link
Copy Markdown
Member

@bbejeck bbejeck commented Mar 17, 2019

This PR is an attempt to fix the RegexSourceIntegrationTest flakiness. My investigation did not reveal any issues, and I was unable to reproduce the failure locally. So I've taken some steps to eliminate what I think could be a race condition in the test.

  1. Delete and create all topics before each test starts.
  2. Give the streams application in each test a unique application ID
  3. Create a KafkaStreams instance local to each test.

If the Jenkins build is all green for this PR, I will create a separate PR for 1.0 and make the same fixes up from 2.0 to trunk
I ran the entire suite of streams tests.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@bbejeck
Copy link
Copy Markdown
Member Author

bbejeck commented Mar 17, 2019

ping @guozhangwang @mjsax @vvcephei @ableegoldman for review

Copy link
Copy Markdown
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bbejeck! This make certainly sense!

We should re-run the build couple of times to see if it fixes the issue (even if not, we should keep the current changes).

STRING_SERDE_CLASSNAME,
STRING_SERDE_CLASSNAME,
properties);
streamsConfiguration = StreamsTestUtils.getStreamsConfig(UUID.randomUUID().toString(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move UUID... to next line

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

@bbejeck
Copy link
Copy Markdown
Member Author

bbejeck commented Mar 18, 2019

Java 10 passed
Java 8 failed unrelated - kafka.admin.DeleteConsumerGroupTest.testConsumptionOnRecreatedTopicAfterTopicWideDeleteInZK
Java 7 failed - the flaky test in question RegexSourceIntegrationTest#testRegexMatchesTopicsAWhenDeleted

Updating per comments and reverting the change from the original cherry-pick.

@bbejeck
Copy link
Copy Markdown
Member Author

bbejeck commented Mar 18, 2019

All tests passed. Kicking off another build to ensure flakiness fixed.

retest this please

Copy link
Copy Markdown
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@bbejeck
Copy link
Copy Markdown
Member Author

bbejeck commented Mar 18, 2019

All tests passed, running again to ensure stability.

retest this please

TestUtils.waitForCondition(new TestCondition() {
@Override
public boolean conditionMet() {
return assignedTopics.equals(expectedFirstAssignment);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious and still have Java7 installed and could reproduce the issue.

The lists are not sorted here and thus the check fails.

The reason is the following exception:

java.lang.UnsupportedOperationException
	at java.util.concurrent.CopyOnWriteArrayList$COWIterator.set(CopyOnWriteArrayList.java:1049)
	at java.util.Collections.sort(Collections.java:159)
	at org.apache.kafka.streams.integration.RegexSourceIntegrationTest$TheConsumerRebalanceListener.onPartitionsAssigned(RegexSourceIntegrationTest.java:445)

Seems CopyOnWriteArrayList cannot be sorted.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Mar 20, 2019

Unrelated failure in Java10 -- Java7 and Java8 passed.

Retest this please.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Mar 20, 2019

Java8 failure unrelated. Java7 and Java10 passed.

Retest this please.

@bbejeck
Copy link
Copy Markdown
Member Author

bbejeck commented Mar 20, 2019

@mjsax I've fixed the ConcurrentModificationException manually, can you take another look?

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Mar 20, 2019

Still +1

Call for second review anyof @guozhangwang @vvcephei @ableegoldman

Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, otherwise lgtm, thanks @bbejeck !

final List<String> assignedTopics = new CopyOnWriteArrayList<>();
streams = new KafkaStreams(builder.build(), streamsConfig, new DefaultKafkaClientSupplier() {
final List<String> assignedTopics = new ArrayList<>();
final KafkaStreams streams = new KafkaStreams(builder.build(), streamsConfig, new DefaultKafkaClientSupplier() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KafkaStreams is AutoCloseable now so you can include its construction inside the try block. Ditto elsewhere.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @guozhangwang this PR is for the 1.1 branch so KafkaStreams does not implement AutoCloseable in branches < 2.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah got it, my bad :)

@bbejeck bbejeck merged commit 92c591d into apache:1.1 Mar 25, 2019
@bbejeck
Copy link
Copy Markdown
Member Author

bbejeck commented Mar 25, 2019

Merged #6459 into 1.1

@bbejeck bbejeck deleted the KAFKA-8026_fix_flaky_regex_source_integration_test branch March 25, 2019 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants