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] [test] Fix flaky test ReplicatorTest #22594

Merged
merged 2 commits into from Apr 29, 2024

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Apr 26, 2024

Motivation

There are two tests that are not normal operations when using Geo-Replication with a global metadata store, which is should be denied in a production pulsar cluster. they are very dangerous, which leads to a lot of topic deletion and makes namespace policies being incorrect, making other tests in the same class flaky

  • Replicatortest.testConfigChange
  • ReplicatorGlobalNSTest .testRemoveLocalClusterOnGlobalNamespace

Modifications

  • Add a description on the class ReplicatorGlobalNSTest to describe it as the class to run no-normal tests.
  • Move Replicatortest.testConfigChange into ReplicatorGlobalNSTest
  • Move the normal test ReplicatorGlobalNSTest. testForcefullyTopicDeletion into Replicatortest

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: x

@lhotari
Copy link
Member

lhotari commented Apr 26, 2024

Rename the class ReplicatorGlobalNSTest to ReplicatorGlobalNSDangerousOperationTest

I think this could be handled with a comment instead. Renaming the file will make following changes harder and I don't think that it provides much clarity. Calling something "dangerous" is very vague and always requires further explanation. That's why comments in the test class or in Pulsar documentation are a better approach to tackle this.

@lhotari
Copy link
Member

lhotari commented Apr 26, 2024

Move Replicatortest.testConfigChange into ReplicatorGlobalNSDangerousOperationTest\nMove the normal test ReplicatorGlobalNSTest. testForcefullyTopicDeletion into Replicatortest

Makes sense

@lhotari
Copy link
Member

lhotari commented Apr 26, 2024

@poorbarcode I disabled one problematic test causing OOME in #22586 . Would it be fine to include the revert in this PR or is it better to handle that separately?

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

@poorbarcode
Copy link
Contributor Author

@poorbarcode I disabled one problematic test causing OOME in #22586 . Would it be fine to include the revert in this PR or is it better to handle that separately?

I will review the test testWriteMarkerTaskOfReplicateSubscriptions that #22586 disabled later, and push another PR to fix the flaky issue

@poorbarcode
Copy link
Contributor Author

Rebase master

@poorbarcode poorbarcode merged commit 6fdc0e3 into apache:master Apr 29, 2024
47 of 49 checks passed
poorbarcode added a commit that referenced this pull request May 7, 2024
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.

None yet

4 participants