Skip to content
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

[Issue 12883] Fix flaky caused by message receive timeout in testBlockBrokerDispatching #12954

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

Jason918
Copy link
Contributor

Fixes #12883

Motivation

As mentioned in #12883 (comment).
In PersistentStreamingDispatcherBlockConsumerTest.testBlockBrokerDispatching, there are still some flaky case other than timeout issue which fixed in #12943

Modifications

  1. For 6 known cases in DispatcherBlockConsumerTest.java:820, change receive(timeout) to receive(). If it does timeout, then test should fail.
  2. For 1 known cases in DispatcherBlockConsumerTest.java:803, change receive(timeout) to receive() won't work. So increase timeout for now.
  3. In case of other potential flaky case caused by receive timeout, increase the base receive timeout from 100ms to 500ms for all cases.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • no-need-doc

Fix flaky.

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 24, 2021
@Jason918
Copy link
Contributor Author

@lhotari PTAL

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari
Copy link
Member

lhotari commented Nov 24, 2021

Thanks for looking into the @Jason918 . I wonder if there's some regression in performance that makes this test fail more often than before. WDYT?

@eolivelli eolivelli merged commit 94736a4 into apache:master Nov 24, 2021
@Jason918
Copy link
Contributor Author

Thanks for looking into the @Jason918 . I wonder if there's some regression in performance that makes this test fail more often than before. WDYT?

I think it's still the metadata warn logs. I fixed in #12896, not merged yet.

@lhotari
Copy link
Member

lhotari commented Nov 24, 2021

I think it's still the metadata warn logs. I fixed in #12896, not merged yet.

@Jason918 Thanks for pointing that out. That indeed is a severe issue in master branch.

eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
@codelipenghui codelipenghui added this to the 2.10.0 milestone Jan 29, 2022
codelipenghui pushed a commit that referenced this pull request Jan 29, 2022
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jan 29, 2022
michaeljmarshall pushed a commit that referenced this pull request Feb 11, 2022
@michaeljmarshall michaeljmarshall added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.3 release/2.9.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky-test: PersistentStreamingDispatcherBlockConsumerTest. testBlockBrokerDispatching
6 participants