-
Notifications
You must be signed in to change notification settings - Fork 5k
[Fix-17854][Worker&SQL Task] Add sql task result alert from worker to alert by rpc #17856
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?
Changes from 11 commits
3581578
55fb13e
80e8080
c948be5
4ad437a
3a3e1dc
505efee
c2de965
b8f407c
cf703c2
83200cc
908c4aa
f618c41
bd75caa
66459ca
5aca48d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
|
|
||
| package org.apache.dolphinscheduler.plugin.task.api; | ||
|
|
||
| import org.apache.dolphinscheduler.common.enums.AlertType; | ||
| import org.apache.dolphinscheduler.plugin.task.api.model.ApplicationInfo; | ||
|
|
||
| public interface TaskCallBack { | ||
|
|
@@ -26,4 +27,6 @@ public interface TaskCallBack { | |
| // todo:The pid should put into runtime context | ||
| @Deprecated | ||
| void updateTaskInstanceInfo(int taskInstanceId); | ||
|
|
||
| void sendAlert(int groupId, String title, String content, AlertType alertType); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't add this at
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, but given the current design, TaskCallBack is still the most straightforward solution for cross-module calls. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,9 +66,9 @@ public class SqlParameters extends AbstractParameters { | |
| private int sqlType; | ||
|
|
||
| /** | ||
| * send email | ||
| * send alert | ||
| */ | ||
| private Boolean sendEmail; | ||
| private Boolean sendAlert; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not break compatibility casually.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
OK, Add database migration to rename JSON key sendEmail to sendAlert |
||
|
|
||
| /** | ||
| * display rows | ||
|
|
@@ -147,12 +147,12 @@ public void setSqlType(int sqlType) { | |
| this.sqlType = sqlType; | ||
| } | ||
|
|
||
| public Boolean getSendEmail() { | ||
| return sendEmail; | ||
| public Boolean getSendAlert() { | ||
| return sendAlert; | ||
| } | ||
|
|
||
| public void setSendEmail(Boolean sendEmail) { | ||
| this.sendEmail = sendEmail; | ||
| public void setSendAlert(Boolean sendAlert) { | ||
| this.sendAlert = sendAlert; | ||
| } | ||
|
|
||
| public int getDisplayRows() { | ||
|
|
@@ -273,7 +273,7 @@ public String toString() { | |
| + ", datasource=" + datasource | ||
| + ", sql='" + sql + '\'' | ||
| + ", sqlType=" + sqlType | ||
| + ", sendEmail=" + sendEmail | ||
| + ", sendAlert=" + sendAlert | ||
| + ", displayRows=" + displayRows | ||
| + ", limit=" + limit | ||
| + ", showType='" + showType + '\'' | ||
|
|
||
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.
Please don't add this into
AbstractTask, this should only used at SqlTask?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.
Excellent. The previously flawed design has been corrected.