Skip to content

MINOR: Increase low timeouts to help with test flakiness#5841

Merged
mjsax merged 3 commits intoapache:2.0from
bbejeck:MINOR_fixes_for_failing_tests_2_0
Oct 26, 2018
Merged

MINOR: Increase low timeouts to help with test flakiness#5841
mjsax merged 3 commits intoapache:2.0from
bbejeck:MINOR_fixes_for_failing_tests_2_0

Conversation

@bbejeck
Copy link
Copy Markdown
Member

@bbejeck bbejeck commented Oct 25, 2018

The InternalTopicIntegrationTest and GlobalThreadShutDownOrderTest use relatively strict timeouts of 5 seconds to check the intermediate state of the tests. The test failures observed in these two tests were not about the final output but asserting the embedded broker sent messages within the given timeframe.

I ran the existing Streams test suite.

Committer Checklist (excluded from commit message)

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

return firstRecordProcessed;
}
}, 5000L, "Has not processed record within 5 seconds");
}, "Has not processed record within 5 seconds");
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.

When no timeout is specified the default time of 15 seconds is used.

@bbejeck
Copy link
Copy Markdown
Member Author

bbejeck commented Oct 25, 2018

@mjsax @vvcephei

\cc @guozhangwang

return firstRecordProcessed;
}
}, 5000L, "Has not processed record within 5 seconds");
}, "Has not processed record within 15 seconds");
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.

When not time specified for the timeout, 15 seconds is used by default

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: what if the default changes :P

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.

With regard to #5840 -- should we set timeout to 30 seconds?

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.

Yeah I'll update this to have the same timeout as well cf #5843

Copy link
Copy Markdown
Contributor

@vvcephei vvcephei 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

@bbejeck
Copy link
Copy Markdown
Member Author

bbejeck commented Oct 25, 2018

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 Oct 26, 2018

updated this

@mjsax mjsax merged commit 79d1bef into apache:2.0 Oct 26, 2018
@mjsax
Copy link
Copy Markdown
Member

mjsax commented Oct 26, 2018

I lost a little track as you have other PR in parallel. This PR was for 2.0 -- what about 2.1 and trunk? Do we need to cherry-pick, or do the other PR cover the other branches?

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Oct 26, 2018

Seems #5843 covers trunk

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Oct 28, 2018

@bbejeck I just merged #5843 -- if this goes into 2.0, I am wondering what we have for 2.1? I assume the timeouts are the same there?

@bbejeck
Copy link
Copy Markdown
Member Author

bbejeck commented Nov 4, 2018

Yes, the timeouts need to be adjusted in 2.1 as well.

I intended to have #5843 cherry-picked into 2.1, but I can issue a separate PR if you prefer me to do that instead.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Nov 5, 2018

Thanks. Cherry-picked #5843 to 2.1 branch.

@bbejeck bbejeck deleted the MINOR_fixes_for_failing_tests_2_0 branch July 10, 2024 13:56
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