Skip to content

KAFKA-8026: Fix for Flaky RegexSourceIntegrationTest#7051

Closed
emschwar wants to merge 1 commit intoapache:2.3from
nexiahome-orig:8026_fix_for_flaky_regexsourceintegrationtest
Closed

KAFKA-8026: Fix for Flaky RegexSourceIntegrationTest#7051
emschwar wants to merge 1 commit intoapache:2.3from
nexiahome-orig:8026_fix_for_flaky_regexsourceintegrationtest

Conversation

@emschwar
Copy link
Copy Markdown

@emschwar emschwar commented Jul 8, 2019

This is a 2.3-idiomatic recreation of Bill Bejeck
bbejeck@gmail.com's original patch for the 1.1 branch, that seems to have been inadvertently omitted. I'm not sure if this PR ought to be against the 2.3 branch, or trunk; it should apply cleanly to either.

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

This change prevents tests from sharing a single KafkaStreams instance across tests, and closes each tests's individual KafkaStreams instance before proceeding to the next test.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

I ran the RegexSourceIntegrationTest 20 times in isolation, and was able to reproduce the flakiness before, and unable to reproduce after this change.

Committer Checklist (excluded from commit message)

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

This is a 2.3-idiomatic recreation of Bill Bejeck
<bbejeck@gmail.com>'s original patch for the 1.1 branch.
@mjsax
Copy link
Copy Markdown
Member

mjsax commented Jul 9, 2019

Thanks for the PR. What do you mean by

that seems to have been inadvertently omitted.

The ticket is resolved already. Why is this PR needed?

@mjsax mjsax added the streams label Jul 9, 2019
@emschwar
Copy link
Copy Markdown
Author

emschwar commented Jul 9, 2019

The changes resolved by the ticket seem to have only been made on the 1.1 branch; when I inspected the 2.3 branch, I didn't see the related changes, and I found that repeatedly running that one specific test showed intermittent failures. After updating Bill Bejeck's original patch to 2.3 (using Duration instead of TimeUtils, etc.), the intermittent failures disappeared, so I thought I'd let y'all know. :)

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Aug 4, 2019

Thanks for the details @emschwar!

@bbejeck can you remember why your PR did no go into trunk ? Seems this PR is porting your fix to 2.3. Hence, I am actually wondering if this fix should be ported to all other branches, too, in particular trunk?

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Sep 4, 2019

@emschwar thanks for the PR. I think the confusion around this PR for 1.1 it that I had to make separate changes is because in Java 7 you can't sort ConcurrenSkipList #6459 (comment).

So to resolve the flaky tests, I used a different approach #6459 (comment)

So for all branches 2.0 > we want to keep the ConcurrentSkipList. Seems when I worked on fixing RegexSourceIntegrationTest the first time there's a couple of other places where I missed updating to ConcurrentSkipList, but that is handled by #7281 (about to get merged).

So I think it's best if we don't merge this PR. But if you found adding the streams.close(Duration.ofSeconds(5)); helps maybe you can update this PR to include only that change.

Thanks for understanding!

\cc @mjsax

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Sep 4, 2019

Thanks @bbejeck! I agree that #7281 (that was cherry-picked to 2.3 branch, too) should actually fix the issue.

Would like to hear from @emschwar if he agrees that this PR can be closed without merging? Seems to be covered by #7281 already.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Sep 12, 2019

Closing this PR as this issue should be covered via #7281

@mjsax mjsax closed this Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants