[Feature-14832][Listener]Implementation of Listener Mechanism #14981

weixiaonan1 · 2023-09-30T08:00:31Z

This closes #14832

Purpose of the pull request

This pull request adds a new type of alert plugin instance: global alert plugin instance. The global alert plugin instance will receive and post all system-generated events by default. Now support 10 types of events.

Brief change log

add a global alert group (id: 2) which users cannot modify/delete, the global alert instance will be added into/ removed from this group automatically.
In addition to the original alarm instance type, add a new option for global alarm instances.
add 10 types of events: ServerDownListenerEvent, ProcessDefinitionCreatedLitenerEvent, ProcessDefinitionUpdatedLitenerEvent, ProcessDefinitionDeletedLitenerEvent, ProcessStartListenerEvent, ProcessEndListenerEvent, ProcessFailListenerEvent, TaskStartListenerEvent, TaskEndListenerEvent, TaskFailListenerEvent. The event will be saved in table t_ds_listener_event.
In addition to AlertBootstrapService, add ListenerEventPostService in AlertServer: get pending listener events from t_ds_listener_event, get global alert instances and post these events.

database change

insert global alert group into t_ds_alertgroup
add instance_type, warning_type in t_ds_alert_plugin_instance. alert instance type: 0 normal; 1 global. warning_type: warning type of normal alert instance which existed in plugin_instance_params previously.
new table t_ds_listener_event: save global listener events, which is simpler than t_ds_alert.

Brief change log

mergeable · 2023-09-30T08:00:37Z

⚠️ This PR do not change database DDL synchronize.

weixiaonan1 · 2023-09-30T10:19:48Z

Due to OSPP time constraints, some improments need to be done in future PRs.

Refine global alert instances: when creating global alert instance, we can choose event types to subscribe to. The current global alert instance subscribes to all events by default. If we can choose event types, there is no longer a need for the default admin warning group. We can create a global alert instance that only subscribe to ServerDownListenerEventto replace the default admin warning group.
Is it necessary to choose warning type for normal alert instances? In order to be compatible with the logic of normal alert instances, we have reserved the option of warning type (success, fail, all). Warning type is actually a collection of event types: success is a collection of ProcessEndEvent; fail is a collection of ProcessFailEvent, ProcessTimeoutEvent, etc. It is necessary for global alert instances to choose event types. However, for normal alert instances, the generation of events is manually triggered by the user (select the alarm strategy and the alarm group bound to the workflow), the types of events generated (ProcessEndEvent, ProcessTimeoutEvent, and ProcessFailEvent) are also fixed. Users have already explicitly bound the alert group and normal alert instances, and the event types which normal alert instance can subscribe to are also fixed, it is fine to send the message directly througn these normal instances. Is it still necessary for users to choose the warning type for normal alert instances?
listener_event and alert: alert content can be abstracted as listener events, and the content stored in t_ds_listener_event is actually alert which global alert group need to process. In this submission, we just add limited logic of global alert events, which is relatively independent with alert. It’s difficult to merge listener_event and alert in this submission because t_ds_alert contains process_definition_code, project_code and other information besides the event itself, And the process of generating alerts in WorkflowExecuteRunnable and ProcessAlertManager is relatively complex. Afterwards, we can consider merging the two.

dolphinscheduler-ui/src/views/security/alarm-instance-manage/detail.tsx

...c/main/java/org/apache/dolphinscheduler/api/service/impl/AlertPluginInstanceServiceImpl.java

+                    globalAlertGroup.setAlertInstanceIds(String.valueOf(alertPluginInstance.getId()));
+                } else {
+                    List<Integer> ids = Arrays.stream(globalAlertGroup.getAlertInstanceIds().split(","))
+                            .map(s -> Integer.parseInt(s.trim()))


...c/main/java/org/apache/dolphinscheduler/api/service/impl/AlertPluginInstanceServiceImpl.java

+            // global instance will be removed from global alert group automatically
+            AlertGroup globalAlertGroup = alertGroupMapper.selectById(2);
+            List<Integer> ids = Arrays.stream(globalAlertGroup.getAlertInstanceIds().split(","))
+                    .map(s -> Integer.parseInt(s.trim()))


...c/main/java/org/apache/dolphinscheduler/api/service/impl/AlertPluginInstanceServiceImpl.java

dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_mysql.sql

dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_postgresql.sql

2. front-end: change to ternary expression 3. back-end: correct license header in ListenerEvent.java

codecov-commenter · 2023-10-22T15:46:52Z

Codecov Report

Merging #14981 (f882ed8) into dev (99214d0) will increase coverage by 0.31%.
The diff coverage is 69.65%.

❗ Current head f882ed8 differs from pull request most recent head ca324ce. Consider uploading reports for the commit ca324ce to get more accurate results

@@             Coverage Diff              @@
##                dev   #14981      +/-   ##
============================================
+ Coverage     37.80%   38.11%   +0.31%     
- Complexity     4596     4643      +47     
============================================
  Files          1248     1262      +14     
  Lines         44711    45155     +444     
  Branches       4912     4944      +32     
============================================
+ Hits          16903    17211     +308     
- Misses        25928    26050     +122     
- Partials       1880     1894      +14

Files	Coverage Δ
...phinscheduler/alert/plugin/AlertPluginManager.java	`3.33% <ø> (+0.89%)`	⬆️
...scheduler/api/controller/AlertGroupController.java	`80.00% <100.00%> (+1.42%)`	⬆️
.../api/controller/AlertPluginInstanceController.java	`100.00% <100.00%> (ø)`
.../org/apache/dolphinscheduler/api/enums/Status.java	`100.00% <100.00%> (ø)`
...lphinscheduler/common/enums/ListenerEventType.java	`100.00% <100.00%> (ø)`
...java/org/apache/dolphinscheduler/dao/AlertDao.java	`26.89% <ø> (ø)`
...y/event/ProcessDefinitionCreatedListenerEvent.java	`100.00% <100.00%> (ø)`
...y/event/ProcessDefinitionUpdatedListenerEvent.java	`100.00% <100.00%> (ø)`
.../master/runner/WorkflowExecuteRunnableFactory.java	`0.00% <ø> (ø)`
...org/apache/dolphinscheduler/alert/AlertServer.java	`0.00% <0.00%> (ø)`
... and 19 more

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

SbloodyS

Generally LGTM. Just some NIT.

...heduler-common/src/main/java/org/apache/dolphinscheduler/common/enums/ListenerEventType.java

...server/src/main/java/org/apache/dolphinscheduler/alert/service/ListenerEventPostService.java

dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_mysql.sql

dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_postgresql.sql

dolphinscheduler-dao/src/main/resources/sql/upgrade/3.3.0_schema/mysql/dolphinscheduler_ddl.sql

...c/main/java/org/apache/dolphinscheduler/api/service/impl/AlertPluginInstanceServiceImpl.java

ruanwenjun · 2023-10-26T06:56:47Z

...src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessDefinitionServiceImpl.java

+            listenerEventAlertManager.publishProcessDefinitionCreatedListenerEvent(loginUser, processDefinition,
+                    taskDefinitionLogs,
+                    taskRelationList);


Can we put this in audit aspect?

Do you mean to add annotation on the methods where generate events to make publish events unified? I think it's hard. Because we cannot get enougn information from arguments and return value of the method for most events such as ProcessStartListenerEvent, ProcessDefinitionDeletedListenerEvent.

sonarqubecloud · 2023-10-28T11:24:33Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
71 Code Smells

69.7% Coverage
18.3% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

zixi0825 · 2023-10-28T13:34:02Z

LGTM

SbloodyS

+1

…#14981) * first commit * 1. sql: sync ddl 2. front-end: change to ternary expression 3. back-end: correct license header in ListenerEvent.java * test case * frontend: remove unnecessary console * fix unit test * remove log depends on user-provided value * fix dolphinscheduler_postgresql.sql * sync database schema * fix unit test * fix unit test * fix some NIT. * extract GLOBAL_ALERT_GROUP_ID into variable * fix ddl bug * add column task_type in t_ds_fav_task in upgrade/3.2.0_schema * add unit test

first commit

ded2331

weixiaonan1 requested review from Amy0104, SbloodyS, caishunfeng, ruanwenjun, songjianet and zhongjiajie as code owners September 30, 2023 08:00

mergeable bot added the sql not sync label Sep 30, 2023

github-actions bot added UI ui and front end related backend labels Sep 30, 2023

songjianet requested changes Oct 7, 2023

View reviewed changes

dolphinscheduler-ui/src/views/security/alarm-instance-manage/detail.tsx Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Oct 7, 2023

View reviewed changes

SbloodyS reviewed Oct 12, 2023

View reviewed changes

dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_mysql.sql Show resolved Hide resolved

dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_postgresql.sql Outdated Show resolved Hide resolved

mergeable bot removed the sql not sync label Oct 12, 2023

1. sql: sync ddl

4d9195d

2. front-end: change to ternary expression 3. back-end: correct license header in ListenerEvent.java

weixiaonan1 force-pushed the dev_14832 branch from f3a3f3c to 4d9195d Compare October 12, 2023 06:08

weixiaonan1 added 2 commits October 12, 2023 23:38

test case

b668769

frontend: remove unnecessary console

cbb79ef

weixiaonan1 requested a review from songjianet October 13, 2023 05:21

weixiaonan1 added 6 commits October 19, 2023 13:42

fix unit test

133ac7a

Merge branch 'dev' into dev_14832

bfc800c

remove log depends on user-provided value

569c441

fix dolphinscheduler_postgresql.sql

6dab1f7

sync database schema

3de8b70

fix unit test

6fce2f3

weixiaonan1 force-pushed the dev_14832 branch from 5a1de0d to 6fce2f3 Compare October 21, 2023 14:38

fix unit test

32593b9

caishunfeng assigned weixiaonan1 Oct 23, 2023

caishunfeng added the feature new feature label Oct 23, 2023

zixi0825 added this to the 3.3.0 milestone Oct 24, 2023

songjianet previously approved these changes Oct 24, 2023

View reviewed changes

Merge branch 'dev' into dev_14832

fb08bb4

SbloodyS added 3.3.0 first time contributor First-time contributor labels Oct 26, 2023

SbloodyS reviewed Oct 26, 2023

View reviewed changes

ruanwenjun reviewed Oct 26, 2023

View reviewed changes

...c/main/java/org/apache/dolphinscheduler/api/service/impl/AlertPluginInstanceServiceImpl.java Outdated Show resolved Hide resolved

ruanwenjun reviewed Oct 26, 2023

View reviewed changes

weixiaonan1 added 2 commits October 27, 2023 22:19

fix some NIT.

23bb662

Merge branch 'dev' into dev_14832

2f18630

weixiaonan1 dismissed songjianet’s stale review via 2f18630 October 27, 2023 14:30

extract GLOBAL_ALERT_GROUP_ID into variable

20b9a46

zixi0825 added the ready-to-merge label Oct 27, 2023

weixiaonan1 added 3 commits October 28, 2023 10:37

fix ddl bug

b2b26e9

add column task_type in t_ds_fav_task in upgrade/3.2.0_schema

2472319

add unit test

ca324ce

SbloodyS approved these changes Oct 29, 2023

View reviewed changes

SbloodyS merged commit c0ed681 into apache:dev Oct 29, 2023

SbloodyS mentioned this pull request Jul 25, 2024

[DSIP-57][Audit] Refactor PR 14981 code #16367

Closed

2 tasks

caishunfeng mentioned this pull request Jul 26, 2024

[DSIP-57][Audit] Revert PR 14981 code #16370

Merged

[Feature-14832][Listener]Implementation of Listener Mechanism #14981

[Feature-14832][Listener]Implementation of Listener Mechanism #14981

Uh oh!

Conversation

weixiaonan1 commented Sep 30, 2023

Purpose of the pull request

Brief change log

database change

Brief change log

Uh oh!

mergeable bot commented Sep 30, 2023

Uh oh!

weixiaonan1 commented Sep 30, 2023

Uh oh!

Uh oh!

Check notice

Check notice

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Oct 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SbloodyS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ruanwenjun Oct 26, 2023

Choose a reason for hiding this comment

Uh oh!

weixiaonan1 Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Oct 28, 2023

Uh oh!

zixi0825 commented Oct 28, 2023

Uh oh!

SbloodyS left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-commenter commented Oct 22, 2023 •

edited

Loading