[Fix-17854] [Worker&SQL Task] Fix SQL task query result alert not being sent

[njnu-seafish]

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. (#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

[njnu-seafish]

@SbloodyS #17854 This issue was automatically closed by the bot due to inactivity. Could a maintainer please reopen it? I've just submitted a more reasonable solution to fix the SQL query task result alerting issue. Thanks so much!

[SbloodyS]

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.

[njnu-seafish]

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.

Good point, your consideration is very thorough. I didn't take the rolling upgrade scenario into account. I have reverted the breaking changes.

[SbloodyS]

Make task-result alert persistence idempotent

TaskExecutorEventListenerImpl#onTaskExecutorSuccess persists the alert immediately after publishing TaskSuccessLifecycleEvent, before the success event has been processed and acknowledged.

Task-executor lifecycle events use at-least-once delivery: the Worker retains and retries a success event until it receives the ACK from TaskSuccessLifecycleEventHandler. If the ACK is delayed or lost, or the Master fails after inserting the alert but before sending the ACK, the same success event is processed again and alertDao.addAlert() inserts another task-result alert. The alert may also be persisted even if the asynchronous success state transition is later rejected.

[njnu-seafish]

Make task-result alert persistence idempotent

TaskExecutorEventListenerImpl#onTaskExecutorSuccess persists the alert immediately after publishing TaskSuccessLifecycleEvent, before the success event has been processed and acknowledged.

Task-executor lifecycle events use at-least-once delivery: the Worker retains and retries a success event until it receives the ACK from TaskSuccessLifecycleEventHandler. If the ACK is delayed or lost, or the Master fails after inserting the alert but before sending the ACK, the same success event is processed again and alertDao.addAlert() inserts another task-result alert. The alert may also be persisted even if the asynchronous success state transition is later rejected.

You've made a really thorough point. That duplication issue can indeed happen, and I've already fixed it.
Make task-result alert persistence idempotent and post-state-transition