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

[SPARK-38241][K8S][TESTS] Close KubernetesClient in K8S integrations tests #35555

Closed

Conversation

martin-g
Copy link
Member

What changes were proposed in this pull request?

Close org.apache.spark.deploy.k8s.integrationtest.backend.IntegrationTestBackend#getKubernetesClient in the cleanUp() of the backend implementations.

Why are the changes needed?

It is a good practice to cleanup resources at the end of the tests, so they do not leak and affect other tests.

Recently I've noticed the following in the output:

===== POSSIBLE THREAD LEAK IN SUITE o.a.s.deploy.k8s.integrationtest.VolcanoSuite, threads: OkHttp [https://192.168.49.2:8443/.](https://192.168.49.2:8443/).. (daemon=false), scala-execution-context-global-26 (daemon=true), OkHttp ConnectionPool (daemon=true), scala-execution-context-global-24 (daemon=true), Okio Watchdog (daemon=true), scala-execution-context-global-23 (daemon=true), scala-execution-context-global-21 (daemon=true), scala-execution-context-global-22 (daemon=true), OkHttp WebSocket [https://192.168.49.2:8443/.](https://192.168.49.2:8443/).. (daemon=false), scala-execution-context-global-25 (daemon=true), scala-execution-context-global-20 (daemon=true) =====

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Run the IT tests

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Copy link
Member

@dongjoon-hyun dongjoon-hyun 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, @martin-g .

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-38241][K8S] Close KubernetesClient in K8S integrations tests [SPARK-38241][K8S][TESTS] Close KubernetesClient in K8S integrations tests Feb 17, 2022
@dongjoon-hyun
Copy link
Member

BTW, Apache Spark community has a convention to use [TESTS] for test-only PRs.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

Copy link
Member

@Yikun Yikun left a comment

Choose a reason for hiding this comment

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

LGTM, it reasonable to close the client after suite.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Sorry folks.

This PR has been blocked due to the recent Docker Desktop validation issue.

We've been very careful because of the previous outage.

@Yikun
Copy link
Member

Yikun commented Feb 22, 2022

@dongjoon-hyun Thanks, it reasonable for careful at this stage.

@martin-g I also tested it in my k8s, it works as expected, but the POSSIBLE THREAD LEAK info are also print after this patch, so we might need to update the PR description.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Merged to master.

@dongjoon-hyun dongjoon-hyun removed their assignment Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants