-
Notifications
You must be signed in to change notification settings - Fork 5k
[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.
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?
Conversation
| // A null warningGroupId indicates that the user has explicitly configured a "no-alert" policy. | ||
| if (workflowInstance.getWarningGroupId() == null) { | ||
| return; | ||
| } |
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.
| // 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
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.
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.
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.
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.
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.
If the user chooses not to send alerts, can this method be called? We need to filter at the upper layer. If the
sendTaskTimeoutAlertmethod ultimately doesn't send an alert, that would be highly unusual.
OK,will handle in TaskTimeoutLifecycleEventHandler
| public void sendTaskTimeoutAlert(WorkflowInstance workflowInstance, | ||
| TaskInstance taskInstance, | ||
| ProjectUser projectUser) { | ||
| assert projectUser != null; |
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.
| 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.
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.
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.
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.
Only a few historical codes contain assert statements.
The proper fix is to set the property to empty, rather than ignore the alert.
|
It seems the issue of the missing workflow timeout alarm function has not been resolved. |
In our environment, the issue has already been resolved—both task timeout alerts and workflow timeout alerts were occurring. NPL exceptions mainly occur in the following two places: |
Of course, we’d be very happy to see any better solutions you might have! |
What version are you using? |
| TaskInstance taskInstance, | ||
| ProjectUser projectUser) { | ||
| TaskInstance taskInstance) { | ||
| ProjectUser projectUser = projectMapper.queryProjectWithUserByWorkflowInstanceId(workflowInstance.getId()); |
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's better to use ProjectDao instead of directly use ProjectMapper.
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's better to use
ProjectDaoinstead of directly useProjectMapper.
ok, it's better
|
|
Please add IT case, if you cannot add IT case ,please at least raise a new issue to supple IT case. |


Purpose of the pull request
close #17820
Brief change log
Add query projectUser logic to avoid NullPointerException
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