[Fix-17854][Worker&SQL Task] Add sql task result alert from worker to alert by rpc

[njnu-seafish]

Purpose of the pull request

close #17854

Brief change log

Add sql task result alert from worker to master by rpc

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]

Hey @ruanwenjun @SbloodyS, whenever you have time, would you mind reviewing this fix to see if it looks reasonable? Really thanks a lot!

[ruanwenjun]

This seems unrelated to the task executor lifecycle, it is a feature of the SQL task.

If the Worker can load the alert plugin, it can send this notification itself. Maybe it's time for us to remove the AlertServer.

[njnu-seafish]

This seems unrelated to the task executor lifecycle, it is a feature of the SQL task.

If the Worker can load the alert plugin, it can send this notification itself. Maybe it's time for us to remove the AlertServer.

Hey, Can task result alerts be considered similar to the YarnAppId generated by a task—i.e., as additional auxiliary events occurring during the task's lifecycle?

Is the intention to deprecate the AlertServer role and migrate all alerting logic to the Worker? If so, note that Workers currently cannot access the database.

[ruanwenjun]

Hey, Can task result alerts be considered similar to the YarnAppId generated by a task—i.e., as additional auxiliary events occurring during the task's lifecycle?

Task results can be treated as part of the task execution context, but this should only store elements like the task result path. It is not suitable for storing SQL output. We should not store task outputs or similar content in the database, as this is highly non-generic, the output might very large.

Is the intention to deprecate the AlertServer role and migrate all alerting logic to the Worker? If so, note that Workers currently cannot access the database.

For such alerts, we do not need to persist them to the database.

[njnu-seafish]

Task results can be treated as part of the task execution context, but this should only store elements like the task result path. It is not suitable for storing SQL output. We should not store task outputs or similar content in the database, as this is highly non-generic, the output might very large.

To prevent excessive resource usage, the size of task result alerts must be constrained—just as we do with log output by limiting the number of displayed lines.

For such alerts, we do not need to persist them to the database.

Alert sending should be asynchronous to avoid blocking the main flow.
Without persistence, alerts could be lost—currently, all alerts are stored in the DB first, and the Alert service processes them from there.

[ruanwenjun]

Here, the alert should be sent synchronously rather than asynchronously.
Otherwise, if the SQL query succeeds but the alert fails, the upper layer cannot know.
If notification delivery fails in this scenario, the SQL execution will be meaningless.

[njnu-seafish]

Here, the alert should be sent synchronously rather than asynchronously. Otherwise, if the SQL query succeeds but the alert fails, the upper layer cannot know. If notification delivery fails in this scenario, the SQL execution will be meaningless.

Hi, I've switched to a synchronous sending approach—could you please review it? @ruanwenjun

[njnu-seafish]

Hey @ruanwenjun @SbloodyS, whenever you have time, please review it. Really thanks a lot!

The previous asynchronous sending alert logic was: Worker → Master → DB → Alert.
A synchronous alert-sending version has now been implemented, with the workflow logic: Worker → Alert.

I have tested both of these approaches in my own environment, and the logs for the synchronous approach are as follows:
1, SQL task log:

2, Worker log:

3,Alert log:

[ruanwenjun]

Currently, only SQL tasks use this; please do not extend this to all task plugins.

[ruanwenjun]

Please don't add this into AbstractTask, this should only used at SqlTask?

[ruanwenjun]

Do not break compatibility casually.

[ruanwenjun]

Don't add this at TaskCallBack, you can see Deprecated at updateTaskInstanceInfo.