Skip to content

[DSIP-35][Alert] Refactor the alert thread model #15932

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.

Sign up for GitHub

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

Jump to bottom
Merged
ruanwenjun merged 5 commits into from
May 9, 2024
Merged

[DSIP-35][Alert] Refactor the alert thread model #15932

ruanwenjun merged 5 commits into from
May 9, 2024

Conversation

@ruanwenjun
Copy link
Member

@ruanwenjun ruanwenjun commented Apr 28, 2024 •
edited
Loading

Purpose of the pull request

close #15931

Brief change log

  • Add EventFetcher、EventPendingQueue、AlertEventLoop interface and implement these.

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)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixAlertSendModel branch from f84b3a2 to 948517b Compare April 28, 2024 13:25
@ruanwenjun ruanwenjun added DSIP improvement make more easy to user or prompt friendly labels Apr 28, 2024
@ruanwenjun ruanwenjun self-assigned this Apr 28, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 28, 2024
View reviewed changes
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixAlertSendModel branch 4 times, most recently from 4016e97 to 51654cf Compare April 29, 2024 01:59
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixAlertSendModel branch 2 times, most recently from 0028796 to fb54c8c Compare April 29, 2024 03:17
@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 36.32385% with 291 lines in your changes are missing coverage. Please review.

Project coverage is 39.93%. Comparing base (bbca37d) to head (6310bb2).

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

Files Patch % Lines
...phinscheduler/alert/service/AbstractEventLoop.java 0.00% 39 Missing ⚠️
...nscheduler/alert/service/AbstractEventFetcher.java 0.00% 37 Missing ⚠️
...scheduler/alert/service/AlertBootstrapService.java 0.00% 29 Missing ⚠️
...inscheduler/alert/service/ListenerEventSender.java 45.83% 24 Missing and 2 partials ⚠️
...inscheduler/alert/service/AbstractEventSender.java 70.93% 19 Missing and 6 partials ⚠️
...r/plugin/registry/zookeeper/ZookeeperRegistry.java 27.58% 16 Missing and 5 partials ⚠️
...inscheduler/plugin/registry/etcd/EtcdRegistry.java 0.00% 18 Missing ⚠️
...he/dolphinscheduler/alert/service/AlertSender.java 68.18% 12 Missing and 2 partials ⚠️
...org/apache/dolphinscheduler/alert/AlertServer.java 0.00% 7 Missing ⚠️
...dolphinscheduler/alert/service/AlertEventLoop.java 0.00% 7 Missing ⚠️
... and 32 more
Additional details and impacted files 
@@             Coverage Diff              @@
##                dev   #15932      +/-   ##
============================================
- Coverage     39.95%   39.93%   -0.03%     
- Complexity     5061     5080      +19     
============================================
  Files          1355     1369      +14     
  Lines         45562    45634      +72     
  Branches       4885     4869      -16     
============================================
+ Hits          18206    18222      +16     
- Misses        25453    25514      +61     
+ Partials       1903     1898       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixAlertSendModel branch 2 times, most recently from a6836ee to cd2fd6e Compare April 29, 2024 11:21
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixAlertSendModel branch 4 times, most recently from 8f6a459 to a0e5210 Compare April 30, 2024 12:29
Radeity
Radeity reviewed May 5, 2024
View reviewed changes
Copy link
Member

@Radeity Radeity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Others LGTM.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixAlertSendModel branch 2 times, most recently from e2d7b6d to 4119a6b Compare May 6, 2024 03:03
@ruanwenjun ruanwenjun requested a review from Radeity May 6, 2024 04:16
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixAlertSendModel branch from 1666693 to d841a14 Compare May 6, 2024 04:18
Radeity
Radeity approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@Radeity Radeity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SbloodyS
Copy link
Member

SbloodyS commented May 8, 2024

It's strange, lack ready-to-merge? image

The input is incorrect, maybe a bug.

It's odd. I can't trigger the re-execution either. It seems to be out of order...

@ruanwenjun
Copy link
Member Author

ruanwenjun commented May 8, 2024

Is there any way we can retrigger the action?
image
I click the Re-run button, but it doesn't work.

I have created an issue to mergeable repo mergeability/mergeable#745, it might be a bug.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixAlertSendModel branch from d841a14 to de22b33 Compare May 8, 2024 03:50
@ruanwenjun ruanwenjun modified the milestone: 3.2.2 May 8, 2024
@ruanwenjun ruanwenjun closed this May 8, 2024
@ruanwenjun ruanwenjun reopened this May 8, 2024
@ruanwenjun
Copy link
Member Author

ruanwenjun commented May 8, 2024

Close/Reopen to retrigger the checker.

@ruanwenjun ruanwenjun added major and removed feature new feature 3.2.2 labels May 8, 2024
@ruanwenjun
Copy link
Member Author

ruanwenjun commented May 8, 2024

We may need to remove this bot, otherwise we need to wait recovery.
image

@SbloodyS
Copy link
Member

SbloodyS commented May 9, 2024

We may need to remove this bot, otherwise we need to wait recovery. image

Agreed. Remove it for now.

@SbloodyS SbloodyS mentioned this pull request May 9, 2024
Merged
@SbloodyS SbloodyS reopened this May 9, 2024
@SbloodyS SbloodyS added the 3.2.2 label May 9, 2024
SbloodyS
SbloodyS approved these changes May 9, 2024
View reviewed changes
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

rickchengx
rickchengx approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@rickchengx rickchengx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 9, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
34.3% Coverage on New Code (required ≥ 60%)

See analysis details on SonarCloud

@ruanwenjun ruanwenjun merged commit 8d336de into apache:dev May 9, 2024
@ruanwenjun ruanwenjun deleted the dev_wenjun_fixAlertSendModel branch May 9, 2024 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@caishunfeng caishunfeng caishunfeng approved these changes

@SbloodyS SbloodyS SbloodyS approved these changes

@rickchengx rickchengx rickchengx approved these changes

@Radeity Radeity Radeity approved these changes

Assignees

@ruanwenjun ruanwenjun

Labels

3.2.2 backend DSIP improvement make more easy to user or prompt friendly major ready-to-merge

Projects

None yet

Milestone

3.2.2

Development

Successfully merging this pull request may close these issues.

[DSIP-35][Alert] Refactor the alert thread model

6 participants

@ruanwenjun @codecov-commenter @Radeity @SbloodyS @caishunfeng @rickchengx