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

[fix][flaky-test]NamespaceServiceTest.flaky/testModularLoadManagerRemoveBundleAndLoad #17487

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Sep 6, 2022

Fixes #17386

Motivation

In this test, we expect it works like this:

  1. persistent broker data to ZK
  2. receive notice: broker data changes, and refresh bundle data in memory
  3. persistent bundle data which already updated in memory to ZK
  4. verify bundle data persist correctly
  5. delete namespace
  6. verify bundle data deleted

But the flow works like this:

  1. persistent broker data to ZK
  2. (High light) race condition
    2-x. receive notice: broker data changes, and refresh bundle data in memory
    2-x. persistent bundle data to ZK ( If the data in memory is not updated, it will not be written to ZK )
  3. ......

Modifications

  • guarantee step-3 is always executed after step-2
  • In this test, the original codes mock ModularLoadManagerWrapper to make isCentralized always return true. But this mock is completely unnecessary because that's ModularLoadManagerWrapper.isCentralized was exactly always returned true, so remove the mock.

Documentation

  • doc-required

  • doc-not-needed

  • doc

  • doc-complete

@poorbarcode
Copy link
Contributor Author

poorbarcode commented Sep 6, 2022

This PR should merge into these branches (because this test was appended in branch-2.11):

  • branch-2.11
  • master

@codelipenghui
Copy link
Contributor

@heesung-sn Please help review this PR, thanks.


private void waitResourceDataUpdateToZK(
LoadManager loadManager, Predicate<LoadData> utilChecker) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the param utilChecker is useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was left over from the previous solution and I forgot to delete it. Already deleted, Thanks

Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

Great work! LGTM

Copy link
Member

@Demogorgon314 Demogorgon314 left a comment

Choose a reason for hiding this comment

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

LGTM.

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

@poorbarcode Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Sep 7, 2022
@Technoboy- Technoboy- removed the doc-not-needed Your PR changes do not impact docs label Sep 7, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 7, 2022
Copy link
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

Better to add some comments for line 809~814

@poorbarcode
Copy link
Contributor Author

Hi @Technoboy-

Better to add some comments for line 809~814

Already add code-comment, thanks

@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

2 similar comments
@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@Technoboy- Technoboy- merged commit 5c67ded into apache:master Sep 8, 2022
Technoboy- pushed a commit that referenced this pull request Sep 8, 2022
@heesung-sn
Copy link
Contributor

LGTM

@poorbarcode poorbarcode deleted the flaky/testModularLoadManagerRemoveBundleAndLoad_ branch September 17, 2022 02:44
@heesung-sn
Copy link
Contributor

heesung-sn commented Mar 4, 2023

  // Wait until "ModularLoadManager" completes processing the ZK notification.
        ModularLoadManagerWrapper modularLoadManagerWrapper = (ModularLoadManagerWrapper) loadManager;
        ModularLoadManagerImpl modularLoadManager = (ModularLoadManagerImpl) modularLoadManagerWrapper.getLoadManager();
        ScheduledExecutorService scheduler = Whitebox.getInternalState(modularLoadManager, "scheduler");

We don't have the scheduler anymore in ModularLoadManagerImpl. I think this test is failing now, caused by #19656

liangyepianzhou pushed a commit that referenced this pull request Jul 6, 2023
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 17, 2023
…oveBundleAndLoad (apache#17487)

(cherry picked from commit 5c67ded)
(cherry picked from commit 44c033c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky-test: NamespaceServiceTest. testModularLoadManagerRemoveBundleAndLoad
7 participants