[Improvement-17723][TaskPlugin] DmsTask resource leak repair #17718
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
|
…er into DSIP-23-DmsTask
thanks , It has been modified and can be reviewed again |
| try { | ||
| return IOUtils.toString(new FileInputStream(filePath), StandardCharsets.UTF_8); | ||
| try (FileInputStream fis = new FileInputStream(filePath)) { | ||
| return IOUtils.toString(fis, StandardCharsets.UTF_8); |
There was a problem hiding this comment.
Use Files.readString is better?
There was a problem hiding this comment.
Should we minimize the changes and close the resources first, and then optimize them later
There was a problem hiding this comment.
Why not just do it all at once? This is not a big change,
| try { | ||
| StopReplicationTaskRequest request = new StopReplicationTaskRequest() | ||
| .withReplicationTaskArn(replicationTaskArn); | ||
| client.stopReplicationTask(request); | ||
| awaitReplicationTaskStatus(STATUS.STOPPED); | ||
| } catch (Exception e) { | ||
| log.error("stopReplicationTask error: ", e); | ||
| } finally { | ||
| if (client != null) { | ||
| // shutdown client | ||
| client.shutdown(); | ||
| } | ||
| } |
There was a problem hiding this comment.
it need to close resources when canceling tasks, right
There was a problem hiding this comment.
We don't need to close resource when canceling, it will be close at handle lifecycle, have you test this? Please at least test in real environment when submit a PR.
3 participants
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