Skip to content

Conversation

@niumy0701
Copy link

@niumy0701 niumy0701 commented Nov 25, 2025

Please refer to the details of main issue #14877
fix #17725

Purpose of the pull request

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

Pull Request Notice

Pull Request Notice

If your pull request contains incompatible change, you should also add it to docs/docs/en/guide/upgrade/incompatible.md

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request addresses resource leak issues in the SagemakerTask plugin by ensuring proper cleanup of the AWS SageMaker client. The fix adds try-catch-finally blocks to trackApplicationStatus() and cancelApplication() methods to guarantee that client.shutdown() is called even when exceptions occur, preventing resource leaks.

  • Adds client cleanup in finally blocks for both trackApplicationStatus() and cancelApplication() methods
  • Adds comprehensive unit tests to verify that client shutdown is called even when exceptions are thrown
  • Changes initPipelineId() visibility from private to public to enable test mocking

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
SagemakerTask.java Wraps trackApplicationStatus() and cancelApplication() logic in try-catch-finally blocks to ensure client.shutdown() is always called, fixing resource leaks
SagemakerTaskTest.java Adds unit tests to verify client.shutdown() is called when exceptions occur in both trackApplicationStatus() and cancelApplication() methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@ruanwenjun
Copy link
Member

Please provide relevant verification before and after the repair.

@ruanwenjun ruanwenjun closed this Dec 9, 2025
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.

[Improvement][TaskPlugin] SagemakerTask resource leak issue

2 participants