Skip to content

[Fix-17004] Fix process termination logic in cancelApplication method #17005

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.

Sign up for GitHub

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

Jump to bottom
Merged
SbloodyS merged 8 commits into from
Feb 23, 2025
Merged

[Fix-17004] Fix process termination logic in cancelApplication method #17005

SbloodyS merged 8 commits into from
Feb 23, 2025

Conversation

@davidzollo
Copy link
Contributor

@davidzollo davidzollo commented Feb 8, 2025 •
edited by SbloodyS
Loading

  • update the ProcessUtils.kill method to ensure it correctly handles process tree termination and direct process destruction.
  • update the AbstractCommandExecutor.cancelApplication method to ensure it correctly handles process termination, including logging and error handling.

close #17004

@davidzollo davidzollo added this to the 3.3.0 milestone Feb 8, 2025
@davidzollo davidzollo added the bug Something isn't working label Feb 10, 2025
ruanwenjun
ruanwenjun reviewed Feb 16, 2025
View reviewed changes
Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

Is this PR generating by AI? This could potentially lead to high code duplicate with other project.

@davidzollo
Copy link
Contributor Author

davidzollo commented Feb 17, 2025

Is this PR generating by AI? This could potentially lead to high code duplicate with other project.

At first, I use cursor generate PR code, but there're many errors it generated, so I have to update a lot to solve this issue.

@apache apache deleted a comment from sonarqubecloud bot Feb 18, 2025
@davidzollo
Copy link
Contributor Author

davidzollo commented Feb 18, 2025

@ruanwenjun Thanks for the review, I have updated the PR, please help review it when you're free, thx

@ruanwenjun
Copy link
Member

ruanwenjun commented Feb 20, 2025

@ruanwenjun Thanks for the review, I have updated the PR, please help review it when you're free, thx

It's better to remove the extra code in #17005 (comment)

@davidzollo
Copy link
Contributor Author

davidzollo commented Feb 20, 2025

@ruanwenjun Thanks for the review, I have updated the PR, please help review it when you're free, thx

It's better to remove the extra code in #17005 (comment)

Thanks for the suggestion, I have removed this logic, please help review it when you're free, thx

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 20, 2025
View reviewed changes
...k-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/AbstractCommandExecutor.java
boolean killed = ProcessUtils.kill(taskRequest);
if (killed) {
log.info("Process tree for task: {} is killed or already finished, pid: {}",
taskRequest.getTaskAppId(), taskRequest.getProcessId());

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
TaskExecutionContext.getTaskAppId
Loading
should be avoided because it has been deprecated.
...k-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/AbstractCommandExecutor.java
taskRequest.getTaskAppId(), taskRequest.getProcessId());
} else {
log.error("Failed to kill process tree for task: {}, pid: {}",
taskRequest.getTaskAppId(), taskRequest.getProcessId());

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
TaskExecutionContext.getTaskAppId
Loading
should be avoided because it has been deprecated.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 20, 2025

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ruanwenjun
ruanwenjun approved these changes Feb 21, 2025
View reviewed changes
Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

LGTM

SbloodyS
SbloodyS approved these changes Feb 23, 2025
View reviewed changes
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

+1

@SbloodyS SbloodyS merged commit ed59a90 into apache:dev Feb 23, 2025
71 checks passed
@DhiyaneshwaranR DhiyaneshwaranR mentioned this pull request Apr 19, 2025
Open
3 tasks
LeonYoah added a commit to LeonYoah/dolphinscheduler that referenced this pull request Nov 1, 2025
@LeonYoah
基于 [Fix-17004] Fix process termination logic in cancelApplication met…
312dfb8 
…hod (apache#17005)

   [Fix-17316][Task-API] Add check process status after killing task (apache#17320)
   [Fix-17436][Workflow]Task timeout kill throw exception (apache#17437  改造支持seatunnel集群模式停止任务
@ruanwenjun ruanwenjun mentioned this pull request Dec 15, 2025
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SbloodyS SbloodyS SbloodyS approved these changes

@ruanwenjun ruanwenjun ruanwenjun approved these changes

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

@Gallardot Gallardot Awaiting requested review from Gallardot Gallardot is a code owner

Assignees

@davidzollo davidzollo

Labels

backend bug Something isn't working priority:high test

Projects

None yet

Milestone

3.3.0-alpha

Development

Successfully merging this pull request may close these issues.

[Bug] [dolphinscheduler-task-plugin] Fix process termination logic in AbstractCommandExecutor.cancelApplication

3 participants

@davidzollo @ruanwenjun @SbloodyS