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 a resource leak in ClusterMigrationTest #21366

Merged

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Oct 14, 2023

Motivation

Running ClusterMigrationTest could fail with OOME.
When investigating the problem, it showed up that ClusterMigrationTest's TestBroker inner class doesn't clean up resources properly. The admin clients weren't closed.

Modifications

  • Call super.internalCleanup() in the cleanup method.
  • Close admin clients in the test cleanup method

Documentation

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

@lhotari lhotari added this to the 3.2.0 milestone Oct 14, 2023
@lhotari lhotari self-assigned this Oct 14, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 14, 2023
@lhotari lhotari force-pushed the lh-fix-clustermigrationtest-resource-leak branch from 119752c to 73d6dae Compare October 15, 2023 06:58
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.

ClusterMigrationTest.testClusterMigrationWithReplicationBacklog

expected [false] but found [true]

Can you help take a look at this flaky test?

@lhotari
Copy link
Member Author

lhotari commented Oct 16, 2023

Can you help take a look at this flaky test?

@Demogorgon314 I'm investigating. It seems to fail consistently after making the change of cleaning up resources properly.

@lhotari lhotari force-pushed the lh-fix-clustermigrationtest-resource-leak branch from 73d6dae to 00ad7de Compare October 16, 2023 08:30
@lhotari
Copy link
Member Author

lhotari commented Oct 16, 2023

@Demogorgon314 PTAL, problem should be fixed now.

@lhotari
Copy link
Member Author

lhotari commented Oct 16, 2023

@vineeth1995 @rdhabalia @vraulji567 I guess we should also consider moving ClusterMigrationTest to it's own test group since the test runtime duration is so long. WDYT?

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thank you!

@tisonkun tisonkun merged commit 39235ed into apache:master Oct 16, 2023
45 of 47 checks passed
lhotari added a commit that referenced this pull request Oct 16, 2023
(cherry picked from commit 39235ed)

# Conflicts:
#	pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ClusterMigrationTest.java
lhotari added a commit that referenced this pull request Oct 16, 2023
vraulji567 pushed a commit to vraulji567/pulsar that referenced this pull request Oct 16, 2023
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
(cherry picked from commit 39235ed)

# Conflicts:
#	pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ClusterMigrationTest.java
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
(cherry picked from commit 39235ed)

# Conflicts:
#	pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ClusterMigrationTest.java
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.

3 participants