Skip to content

[Fix-17820][Master] Fix task timeout alerts failed #17818

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

Merged
SbloodyS merged 5 commits into from
Jan 5, 2026
Merged

[Fix-17820][Master] Fix task timeout alerts failed #17818

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1b7c3b7
add query ProjectUser
Dec 23, 2025
0daf224
update the logic when warningGroupId is null
Dec 24, 2025
dddb5c2
Merge branch 'dev' into Fix-17817
njnu-seafish Dec 24, 2025
7ecf734
query by projectDao
Dec 24, 2025
5812348
Merge branch 'dev' into Fix-17817
SbloodyS Jan 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/AlertDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,13 @@ private void saveTaskTimeoutAlert(Alert alert, String content, int alertGroupId)
public void sendTaskTimeoutAlert(WorkflowInstance workflowInstance,
TaskInstance taskInstance,
ProjectUser projectUser) {
assert projectUser != null;
Copy link
Member

@ruanwenjun ruanwenjun Dec 23, 2025

Choose a reason for hiding this comment

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

Suggested change
assert projectUser != null;
if(projectUser == null) {
throw IlleganArguementException("project user is null");
}

assert will do nothing if the jvm doesn't enable it.

And I don't think we need to check the project user here, since this shouldn't affect the alert, alert only need to take care about alert plugin, if the project user is null, it only affects the alert content.

Copy link
Contributor Author

@njnu-seafish njnu-seafish Dec 24, 2025

Choose a reason for hiding this comment

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

if(projectUser == null) {
throw IlleganArguementException("project user is null");
}

The DS project contains a large number of assert statements—I assumed they were recommended for use.
If we don't check in advance, a NullPointerException (NPE) will occur.

Copy link
Member

@ruanwenjun ruanwenjun Dec 24, 2025

Choose a reason for hiding this comment

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

Only a few historical codes contain assert statements.

The proper fix is to set the property to empty, rather than ignore the alert.


// A null warningGroupId indicates that the user has explicitly configured a "no-alert" policy.
if (workflowInstance.getWarningGroupId() == null) {
return;
}
Copy link
Member

@ruanwenjun ruanwenjun Dec 23, 2025

Choose a reason for hiding this comment

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

Suggested change
// A null warningGroupId indicates that the user has explicitly configured a "no-alert" policy.
if (workflowInstance.getWarningGroupId() == null) {
return;
}

Don't add this kind of code, we needn't check at here, the method name is not sendTaskTimeoutAlertIfNeeded

Copy link
Contributor Author

@njnu-seafish njnu-seafish Dec 24, 2025

Choose a reason for hiding this comment

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

Don't add this kind of code, we needn't check at here, the method name is not sendTaskTimeoutAlertIfNeeded

If the user chooses not to send alerts, workflowInstance.getWarningGroupId() will be null, which will cause a NullPointerException.

Copy link
Member

@ruanwenjun ruanwenjun Dec 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If the user chooses not to send alerts, can this method be called? We need to filter at the upper layer.
If the sendTaskTimeoutAlert method ultimately doesn't send an alert, that would be highly unusual.

Copy link
Contributor Author

@njnu-seafish njnu-seafish Dec 24, 2025

Choose a reason for hiding this comment

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

If the user chooses not to send alerts, can this method be called? We need to filter at the upper layer. If the sendTaskTimeoutAlert method ultimately doesn't send an alert, that would be highly unusual.

OK,will handle in TaskTimeoutLifecycleEventHandler


Alert alert = new Alert();
List<WorkflowAlertContent> workflowAlertContentList = new ArrayList<>(1);
WorkflowAlertContent workflowAlertContent = WorkflowAlertContent.builder()
Expand Down
3 changes: 1 addition & 2 deletions ...heduler/server/master/engine/task/lifecycle/handler/TaskTimeoutLifecycleEventHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,7 @@ private void doTaskTimeoutKill(final ITaskExecutionRunnable taskExecutionRunnabl
private void doTaskTimeoutAlert(final ITaskExecutionRunnable taskExecutionRunnable) {
final WorkflowInstance workflowInstance = taskExecutionRunnable.getWorkflowInstance();
final TaskInstance taskInstance = taskExecutionRunnable.getTaskInstance();
// todo: inject the projectUser
workflowAlertManager.sendTaskTimeoutAlert(workflowInstance, taskInstance, null);
workflowAlertManager.sendTaskTimeoutAlert(workflowInstance, taskInstance);
}

@Override
Expand Down
8 changes: 6 additions & 2 deletions ...service/src/main/java/org/apache/dolphinscheduler/service/alert/WorkflowAlertManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.apache.dolphinscheduler.dao.entity.WorkflowAlertContent;
import org.apache.dolphinscheduler.dao.entity.WorkflowDefinitionLog;
import org.apache.dolphinscheduler.dao.entity.WorkflowInstance;
import org.apache.dolphinscheduler.dao.mapper.ProjectMapper;
import org.apache.dolphinscheduler.dao.repository.ProjectDao;
import org.apache.dolphinscheduler.dao.repository.UserDao;
import org.apache.dolphinscheduler.dao.repository.WorkflowDefinitionLogDao;
Expand Down Expand Up @@ -60,6 +61,9 @@ public class WorkflowAlertManager {
@Autowired
private ProjectDao projectDao;

@Autowired
private ProjectMapper projectMapper;

/**
* convert command type to human-readable name
*
Expand Down Expand Up @@ -260,8 +264,8 @@ public boolean isNeedToSendWarning(WorkflowInstance workflowInstance) {
}

public void sendTaskTimeoutAlert(WorkflowInstance workflowInstance,
TaskInstance taskInstance,
ProjectUser projectUser) {
TaskInstance taskInstance) {
ProjectUser projectUser = projectMapper.queryProjectWithUserByWorkflowInstanceId(workflowInstance.getId());
Copy link
Member

@SbloodyS SbloodyS Dec 24, 2025

Choose a reason for hiding this comment

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

It's better to use ProjectDao instead of directly use ProjectMapper.

Copy link
Contributor Author

@njnu-seafish njnu-seafish Dec 24, 2025

Choose a reason for hiding this comment

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

It's better to use ProjectDao instead of directly use ProjectMapper.

ok, it's better

alertDao.sendTaskTimeoutAlert(workflowInstance, taskInstance, projectUser);
}
}