njnu-seafish opened a new pull request, #18549:
URL: https://github.com/apache/dolphinscheduler/pull/18549
<!--Thanks very much for contributing to Apache DolphinScheduler, we are
happy that you want to help us improve DolphinScheduler! -->
## Was this PR generated or assisted by AI?
Yes, I design the architecture and write the core code myself, then use an
LLM to review and optimize the logic.
## Purpose of the pull request
close #17854
## Brief change log
### Purpose
Fix #17854: the SQL task "query result" alert feature silently stopped
working after the
task-executor refactoring (DSIP-73). The alert flag and payload (`needAlert`
/
`taskAlertInfo`) used to live on `AbstractTask`, but no component consumed
them anymore,
so enabling "Send Alert" on a SQL task had no effect.
### Root cause
The `needAlert` / `taskAlertInfo` fields were only defined and set in the
task plugin
(`AbstractTask`), while the Master never read them. After the task-executor
module
refactor, the success lifecycle event did not carry the alert information to
the Master,
so the alert was never persisted/sent.
### What changed
- **Task plugin side**
- Moved `needAlert` / `taskAlertInfo` from `AbstractTask` into
`TaskExecutionContext`
so they can be carried across the Worker -> Master RPC.
- `SqlTask`: prepare the alert info (`title`, `alertGroupId`,
`AlertType.TASK_RESULT`)
and truncate the query result to `displayRows` (default if unset) to
avoid oversized
RPC payloads; empty result sets are also covered.
- Renamed the SQL task parameter `sendEmail` to `sendAlert` (kept
`@JsonAlias("sendEmail")`
for backward-compatible deserialization) and removed the obsolete
`showType` field.
- **Event / Master**
- `TaskExecutorSuccessLifecycleEvent` now carries `needAlert` and
`taskAlertInfo`.
- `TaskExecutorEventListenerImpl` consumes the success event: when
`needAlert` is true
and a valid `alertGroupId` is present, it delegates to
`WorkflowAlertManager.sendTaskResultAlert`
(with project / workflow / task context filled in); otherwise it logs a
warning instead
of silently dropping the alert.
- **Alert chain**
- Added `AlertType.TASK_RESULT (8)`.
- `AlertSendRequest` now carries `AlertType` instead of a plain int
`warnType`;
`AlertSender.syncHandler` and `AlertOperatorImpl` propagate it into
`AlertData`.
- **Data migration & docs**
- Upgrade DML for MySQL / PostgreSQL migrates `sendEmail` -> `sendAlert` in
`t_ds_task_definition` and `t_ds_task_definition_log` (null-safe guards
added).
- Documented the incompatible change in `incompatible.md` (en/zh).
### Verification
- Unit tests added/updated:
- `SqlParametersTest`: JSON backward compatibility (`sendEmail` ->
`sendAlert`) and
new field name.
- `AlertSenderTest`: `syncHandler` with the new `AlertType` argument.
- Local build of the touched modules passes (`mvn compile`).
I previously submitted a PR proposing that the Worker role should directly
send RPC requests to the Master to transmit SQL result set alerts. The
proposal was rejected. (https://github.com/apache/dolphinscheduler/pull/17856)
## Verify this pull request
<!--*(Please pick either of the following options)*-->
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:
<!--*(example:)*
- *Added dolphinscheduler-dao tests for end-to-end.*
- *Added CronUtilsTest to verify the change.*
- *Manually verified the change by testing locally.* -->
(or)
## Pull Request Notice
[Pull Request
Notice](https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)
If your pull request contains incompatible change, you should also add it to
`docs/docs/en/guide/upgrade/incompatible.md`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
