-
Notifications
You must be signed in to change notification settings - Fork 5k
[Improvement-17723][TaskPlugin] DmsTask resource leak repair #17718
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
base: dev
Are you sure you want to change the base?
Changes from all commits
cdee83e
3394bb2
ed00657
451bf9f
80b1ee7
6de20c9
cd182de
a08701a
2929213
03c9b3f
8c3d2d7
7be50e7
56115d3
f61aadc
4137abb
f905728
f259d12
1718eb2
5eac924
0b04ffe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,10 +141,19 @@ public void stopReplicationTask() { | |
| if (replicationTaskArn == null) { | ||
| return; | ||
| } | ||
| StopReplicationTaskRequest request = new StopReplicationTaskRequest() | ||
| .withReplicationTaskArn(replicationTaskArn); | ||
| client.stopReplicationTask(request); | ||
| awaitReplicationTaskStatus(STATUS.STOPPED); | ||
| 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(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public Boolean deleteReplicationTask() { | ||
|
|
@@ -268,8 +277,8 @@ public String replaceFileParameters(String parameter) throws IOException { | |
| } | ||
| if (parameter.startsWith("file://")) { | ||
| String filePath = parameter.substring(7); | ||
| try { | ||
| return IOUtils.toString(new FileInputStream(filePath), StandardCharsets.UTF_8); | ||
| try (FileInputStream fis = new FileInputStream(filePath)) { | ||
| return IOUtils.toString(fis, StandardCharsets.UTF_8); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use Files.readString is better?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we minimize the changes and close the resources first, and then optimize them later
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just do it all at once? This is not a big change, |
||
| } catch (IOException e) { | ||
| throw new IOException("Error reading file: " + filePath, e); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it need to close resources when canceling tasks, right
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.