Skip to content
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

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

Merged
merged 5 commits into from
May 9, 2024

Conversation

ruanwenjun
Copy link
Member

@ruanwenjun ruanwenjun commented Apr 28, 2024

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 added DSIP improvement make more easy to user or prompt friendly labels Apr 28, 2024
@ruanwenjun ruanwenjun self-assigned this Apr 28, 2024
@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

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
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
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

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

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

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 added the 3.2.2 label May 9, 2024
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

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

Copy link

sonarcloud 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
61 of 63 checks passed
@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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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