-
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
Changes from 19 commits
cdee83e
3394bb2
ed00657
451bf9f
80b1ee7
6de20c9
cd182de
a08701a
2929213
03c9b3f
8c3d2d7
7be50e7
56115d3
f61aadc
4137abb
f905728
f259d12
1718eb2
5eac924
0b04ffe
ff93f77
e1af574
e8175f5
9c8b7f0
7809aa3
5a12213
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 |
|---|---|---|
|
|
@@ -24,9 +24,11 @@ | |
|
|
||
| import org.apache.dolphinscheduler.common.utils.JSONUtils; | ||
| import org.apache.dolphinscheduler.plugin.task.api.AbstractRemoteTask; | ||
| import org.apache.dolphinscheduler.plugin.task.api.TaskCallBack; | ||
| import org.apache.dolphinscheduler.plugin.task.api.TaskConstants; | ||
| import org.apache.dolphinscheduler.plugin.task.api.TaskException; | ||
| import org.apache.dolphinscheduler.plugin.task.api.TaskExecutionContext; | ||
| import org.apache.dolphinscheduler.plugin.task.api.model.ApplicationInfo; | ||
| import org.apache.dolphinscheduler.plugin.task.api.utils.ParameterUtils; | ||
|
|
||
| import org.apache.commons.beanutils.BeanUtils; | ||
|
|
@@ -78,6 +80,42 @@ public void init() throws TaskException { | |
| initDmsHook(); | ||
| } | ||
|
|
||
| /** | ||
| * If appIds is empty, submit a new remote application; otherwise, just track application status. | ||
| * | ||
| * @param taskCallBack | ||
| * @throws TaskException | ||
| */ | ||
| @Override | ||
| public void handle(TaskCallBack taskCallBack) throws TaskException { | ||
| try { | ||
| // 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(); | ||
|
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. AWS DMS task types will not be submitted to yarn or k8s for execution, so this logic should not be added.
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.
DMSTask also executes the handle method. When rewriting the handle method, I added the logic to close resources
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. The two do not conflict or even have nothing to do with each other.
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.
Do you mean that DMS task does not involve resource leakage?
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. Yes. We've already safely close connection in
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.
Based on the previous discussion with @ruanwenjun , it is necessary to uniformly close resources in the handle method.
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. @niumy0701 For such issues, please first provide evidence of resource leakage.
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.
okay,I am trying to simulate a resource leakage |
||
| } finally { | ||
| // shutdown dmsHook client | ||
| if (dmsHook.getClient() != null) { | ||
| dmsHook.getClient().shutdown(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public List<String> getApplicationIds() throws TaskException { | ||
| return Collections.emptyList(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.