Repository navigation
[Fix-17854] [Worker&SQL Task] Fix SQL task query result alert not being sent - #18549
njnu-seafish wants to merge 65 commits into
Conversation
…r into Fix-17854-2
# Conflicts: # docs/docs/en/guide/upgrade/incompatible.md # docs/docs/zh/guide/upgrade/incompatible.md
SbloodyS
changed the title
SbloodyS
left a comment
SbloodyS
left a comment
There was a problem hiding this comment.
Preserve the existing sendEmail field name
SqlParameters renames the persisted/API field from sendEmail to sendAlert, and the UI now only reads/writes sendAlert.
Although @JsonAlias("sendEmail") keeps deserialization compatible, serialization and UI payloads use the new name. This can cause existing clients, SDKs, integrations, or mixed-version components to silently lose the setting. It also introduces an unnecessary database migration and an incompatible public contract change.
Please keep sendEmail as the field name and only update the semantic description/UI label to indicate that alerts can use channels other than email. If the rename is still required, please provide an explicit compatibility strategy covering API clients, UI loading of legacy definitions, and rolling upgrades.
Keep AlertSendRequest wire-compatible
AlertSendRequest changes warnType: int to alertType: AlertType.
This changes both the field name and the serialized type of the Master–Alert RPC request. During a rolling upgrade, an old Alert Server will still expect warnType, while a new Alert Server may receive a request without alertType from an old Master. The result can be a default/incorrect alert type or a NullPointerException at alertType.getCode().
Please retain the existing warnType field for compatibility, or support both fields with explicit conversion and add a mixed-version serialization test.
| <insert id="insertTaskResultAlertIfAbsent"> | ||
| INSERT INTO t_ds_alert(sign, title, content, alert_status, warning_type, log, alertgroup_id, | ||
| create_time, update_time, project_code, workflow_definition_code, | ||
| workflow_instance_id, alert_type) | ||
| SELECT #{alert.sign}, #{alert.title}, #{alert.content}, #{alert.alertStatus.code}, | ||
| #{alert.warningType.code}, #{alert.log}, #{alert.alertGroupId}, #{alert.createTime}, | ||
| #{alert.updateTime}, #{alert.projectCode}, #{alert.workflowDefinitionCode}, | ||
| #{alert.workflowInstanceId}, #{alert.alertType.code} | ||
| WHERE NOT EXISTS ( | ||
| SELECT 1 FROM t_ds_alert | ||
| WHERE sign = #{alert.sign} | ||
| AND workflow_instance_id = #{alert.workflowInstanceId} | ||
| AND alert_type = #{alert.alertType.code} | ||
| ) | ||
| </insert> |
There was a problem hiding this comment.
Such logic should not be handled in the database, but should be handled by code.
There was a problem hiding this comment.
Such logic should not be handled in the database, but should be handled by code.
Thanks for the feedback. I've moved the dedup logic out of SQL and into application code.
The INSERT ... SELECT ... WHERE NOT EXISTS statement has been replaced with a plain INSERT ... VALUES. Idempotency is now handled entirely in code: AlertDao#addTaskResultAlert performs a direct insert and catches DuplicateKeyException (thrown by the uk_alert_dedup unique constraint) to treat the duplicate as a skip. The unique constraint remains as the DB-level safety net, but no dedup logic lives in the SQL itself.
| DELETE FROM t_ds_alert a | ||
| USING t_ds_alert b | ||
| WHERE a.id < b.id | ||
| AND a.sign = b.sign | ||
| AND a.workflow_instance_id = b.workflow_instance_id | ||
| AND a.alert_type = b.alert_type; |
There was a problem hiding this comment.
Deleting user data is a very dangerous action, and we should find a better compatible way.
There was a problem hiding this comment.
Deleting user data is a very dangerous action, and we should find a better compatible way.
done.
The upgrade now only adds the unique index without touching any existing data. If duplicate rows already exist, the index creation will fail — the user can review and resolve them manually.
| -- Enforce idempotent task-result alerts at the database level. | ||
| -- Allows INSERT IGNORE (MySQL) / ON CONFLICT DO NOTHING (PostgreSQL) to atomically | ||
| -- prevent duplicates without check-then-insert race conditions. | ||
| -- Clean up any existing duplicate rows before adding the unique constraint. |
There was a problem hiding this comment.
You should check for yourself to avoid invalid comments generated by AI.
There was a problem hiding this comment.
You should check for yourself to avoid invalid comments generated by AI.
sorry, it have been simplified to a single line.
|
@SbloodyS When you have a moment, could you please review the code again? Thanks so much! |
… with uk_alert_dedup constraint
There was a problem hiding this comment.
Keep the log display limit separate from alert content.
In SqlTask.java, lines 337–344, displayRows now truncates the notification payload. The UI labels this setting “Log display,” and the previous implementation passed the complete result to the alert. With 50 result rows and displayRows=10, recipients now receive only 10 rows without any truncation notice. Please preserve the full alert result or introduce a separate, explicit alert limit.
The compatibility bridge ignores the retained protected fields.
In AbstractTask.java, lines 133–164, getters and setters use only taskRequest, while needAlert and taskAlertInfo remain independently writable protected fields. An existing subclass that assigns these fields directly now gets false/null from the getters, and its alert data never reaches the success event. Please synchronize legacy field writes into the context before reporting task completion.
Thanks for the review — both points are valid and have been fixed in commit 004a7c5f0.
Agreed. displayRows is only a log-display setting, so reusing it as the alert payload limit was a silent behavior change. The truncation has been removed from prepareTaskResultAlert: the alert now carries the complete query result (empty result sets still get a placeholder row, as before). The payload stays bounded by the query limit (QUERY_LIMIT, default 10000).
The @deprecated getters/setters in AbstractTask now merge the legacy fields with the context: getNeedAlert() returns the field OR the context value, getTaskAlertInfo() prefers the field and falls back to the context, and both setters write through to both locations. On the executor side, PhysicalTaskExecutor.doTrackTaskPluginStatus() synchronizes these legacy values into the TaskExecutionContext once the task reaches SUCCEEDED — right before the success lifecycle event is built — so alert info written by third-party plugins via direct field assignment is no longer lost. Tests added/updated: AbstractTaskTest covers direct field assignment, setter write-through, and context fallback; SqlTaskTest now asserts the alert content keeps all rows regardless of displayRows. All tests pass (AbstractTaskTest 6/6, SqlTaskTest 27/27). |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Synchronize the legacy needAlert/taskAlertInfo fields written directly | ||
| // by AbstractTask subclasses into the context, so that the success | ||
| // lifecycle event carries the alert info of third-party plugins. | ||
| taskExecutionContext.setNeedAlert(physicalTask.getNeedAlert()); |
| // by AbstractTask subclasses into the context, so that the success | ||
| // lifecycle event carries the alert info of third-party plugins. | ||
| taskExecutionContext.setNeedAlert(physicalTask.getNeedAlert()); | ||
| taskExecutionContext.setTaskAlertInfo(physicalTask.getTaskAlertInfo()); |
SbloodyS
left a comment
SbloodyS
left a comment
There was a problem hiding this comment.
Do not persist result alerts for rejected success events
TaskSuccessLifecycleEventHandler.java:56–60 checks only needAlert after calling onSucceedEvent(). However, TaskPauseStateAction and TaskKillStateAction merely log and ignore a late success event; they return normally without transitioning the task to SUCCESS. The handler still persists a TASK_RESULT alert for the paused/killed task.
Gate alert persistence on the effective SUCCESS state, while preserving idempotent handling of repeated success events. Add handler-level regression tests for rejected late success events and accepted/repeated success events; the current SQL preparation and DAO tests do not cover this behavior.
Good catch. The success-event handler now gates alert persistence on the effective task state, fixed in 15c18bd.
|
|
|
||
| When performing a rolling upgrade, **the Alert Server must be upgraded before Master/Worker**. | ||
|
|
||
| Starting from 3.5.0, the Master may persist new `alert_type` enum values (e.g. `TASK_RESULT`) into the `t_ds_alert` table. If the Alert Server has not yet been upgraded to a version that includes the new enum value, MyBatis will deserialize the unknown value as `null`, causing `AlertSender` to throw a `NullPointerException` while building the alert data. The alert will remain stuck in `WAIT_EXECUTION` and never be delivered. |
There was a problem hiding this comment.
If the upgrade fails, users need to be provided with specific operation steps.
There was a problem hiding this comment.
If the upgrade fails, users need to be provided with specific operation steps.
Thanks for the feedback. Fixed in be65492 — the upgrade doc now provides concrete recovery steps for a failed upgrade.
The new "If the upgrade fails" section (after the Rolling Upgrade Order) covers the case where Master/Worker were already upgraded before the Alert Server and new alert_type values were persisted to t_ds_alert:
Upgrade the Alert Server to a version that includes the new enum values and restart it — alerts stuck in WAIT_EXECUTION will be picked up and delivered normally.
If the Alert Server cannot be upgraded immediately, stop the old Alert Server first, then back up and clean up the affected rows (create table t_ds_alert_bak as select * from t_ds_alert where alert_type = 8; followed by delete from t_ds_alert where alert_type = 8;), so they cannot keep blocking alert delivery.
The Chinese upgrade doc is updated in sync.
This branch has not been deployed
3 participants
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 onAbstractTask, but no component consumed them anymore,so enabling "Send Alert" on a SQL task had no effect.
Root cause
The
needAlert/taskAlertInfofields were only defined and set in the task plugin(
AbstractTask), while the Master never read them. After the task-executor modulerefactor, 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
needAlert/taskAlertInfofromAbstractTaskintoTaskExecutionContextso 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 oversizedRPC payloads; empty result sets are also covered.
sendEmailtosendAlert(kept@JsonAlias("sendEmail")for backward-compatible deserialization) and removed the obsolete
showTypefield.Event / Master
TaskExecutorSuccessLifecycleEventnow carriesneedAlertandtaskAlertInfo.TaskExecutorEventListenerImplconsumes the success event: whenneedAlertis trueand a valid
alertGroupIdis present, it delegates toWorkflowAlertManager.sendTaskResultAlert(with project / workflow / task context filled in); otherwise it logs a warning instead
of silently dropping the alert.
Alert chain
AlertType.TASK_RESULT (8).AlertSendRequestnow carriesAlertTypeinstead of a plain intwarnType;AlertSender.syncHandlerandAlertOperatorImplpropagate it intoAlertData.Data migration & docs
sendEmail->sendAlertint_ds_task_definitionandt_ds_task_definition_log(null-safe guards added).incompatible.md(en/zh).Verification
SqlParametersTest: JSON backward compatibility (sendEmail->sendAlert) andnew field name.
AlertSenderTest:syncHandlerwith the newAlertTypeargument.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. (#17856)
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