[Improvement-17723][TaskPlugin] DmsTask resource leak repair#17718
[Improvement-17723][TaskPlugin] DmsTask resource leak repair#17718niumy0701 wants to merge 26 commits intoapache:devfrom
Conversation
niumy0701
requested review from
Gallardot,
SbloodyS and
caishunfeng
as code owners
niumy0701
changed the title
niumy0701
changed the title
done |
There was a problem hiding this comment.
Pull request overview
This pull request addresses resource leaks in the DMS task plugin by adding proper resource cleanup for AWS DMS clients and fixing a FileInputStream resource leak. However, the implementation introduces a critical bug in the checkFinishedReplicationTask() method where the client is shut down before it's fully used.
Key changes:
- Added
client.shutdown()calls to three methods (checkFinishedReplicationTask,stopReplicationTask,deleteReplicationTask) to prevent AWS client resource leaks - Fixed FileInputStream resource leak in
replaceFileParametersusing try-with-resources pattern - Added comprehensive test coverage for the
replaceFileParametersmethod
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| DmsHook.java | Added client shutdown calls to cleanup AWS DMS client resources; fixed FileInputStream leak with try-with-resources |
| DmsHookTest.java | Added four new test cases for replaceFileParameters method covering null input, normal strings, existing files, and non-existent files |
💡 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.
...cheduler-task-dms/src/test/java/org/apache/dolphinscheduler/plugin/task/dms/DmsHookTest.java
Show resolved
Hide resolved
...cheduler-task-dms/src/test/java/org/apache/dolphinscheduler/plugin/task/dms/DmsHookTest.java
Show resolved
Hide resolved
...cheduler-task-dms/src/test/java/org/apache/dolphinscheduler/plugin/task/dms/DmsHookTest.java
Outdated
Show resolved
Hide resolved
...hinscheduler-task-dms/src/main/java/org/apache/dolphinscheduler/plugin/task/dms/DmsHook.java
Outdated
Show resolved
Hide resolved
ruanwenjun
left a comment
ruanwenjun
left a comment
There was a problem hiding this comment.
You need to add close method in DmsHook and close the hook in upper layer, if there throw any exception, still exist resource leak
thanks , It has been modified and can be reviewed again |
...hinscheduler-task-dms/src/main/java/org/apache/dolphinscheduler/plugin/task/dms/DmsHook.java
Show resolved
Hide resolved
...hinscheduler-task-dms/src/main/java/org/apache/dolphinscheduler/plugin/task/dms/DmsHook.java
Outdated
Show resolved
Hide resolved
This reverts commit e1af574.
| // if appIds is not empty, just track application status, avoid resubmitting remote task | ||
| if (StringUtils.isNotEmpty(taskRequest.getAppIds())) { | ||
| setAppIds(taskRequest.getAppIds()); | ||
| trackApplicationStatus(); | ||
| return; | ||
| } | ||
|
|
||
| // submit a remote application | ||
| submitApplication(); | ||
|
|
||
| if (StringUtils.isNotEmpty(getAppIds())) { | ||
| taskRequest.setAppIds(getAppIds()); | ||
| // callback to update remote application info | ||
| taskCallBack.updateRemoteApplicationInfo(taskRequest.getTaskInstanceId(), | ||
| new ApplicationInfo(getAppIds())); | ||
| } | ||
|
|
||
| // keep tracking application status | ||
| trackApplicationStatus(); |
There was a problem hiding this comment.
AWS DMS task types will not be submitted to yarn or k8s for execution, so this logic should not be added.
There was a problem hiding this comment.
AWS DMS task types will not be submitted to yarn or k8s for execution, so this logic should not be added.
DMSTask also executes the handle method. When rewriting the handle method, I added the logic to close resources
There was a problem hiding this comment.
The two do not conflict or even have nothing to do with each other.
There was a problem hiding this comment.
The two do not conflict or even have nothing to do with each other.
Do you mean that DMS task does not involve resource leakage?
There was a problem hiding this comment.
Yes. We've already safely close connection in cancelApplication
There was a problem hiding this comment.
Yes. We've already safely close connection in
cancelApplication
Based on the previous discussion with @ruanwenjun , it is necessary to uniformly close resources in the handle method.
Shall we discuss and confirm again
There was a problem hiding this comment.
@niumy0701 For such issues, please first provide evidence of resource leakage.
There was a problem hiding this comment.
@niumy0701 For such issues, please first provide evidence of resource leakage.
okay,I am trying to simulate a resource leakage
|
This reverts commit 7809aa3.
|
Please provide relevant verification before and after the repair. |
Please refer to the details of main issue #14877
fix #17723
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