Skip to content

Conversation

@ruanwenjun
Copy link
Member

@ruanwenjun ruanwenjun commented Apr 4, 2023

Purpose of the pull request

Right now, we can only support start single alert server, and need to write the alert server ip in worker's application.yml.

This will have some problems:

  • Once the single alert server crash the alert cannot use.
  • Once the alert server ip changed, we need to update the worker's application.yml and restart the worker.

Brief change log

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 changed the title Support alert server cluster Support alert server HA Apr 4, 2023
@ruanwenjun ruanwenjun added feature new feature 3.2.0 for 3.2.0 version labels Apr 4, 2023
@ruanwenjun ruanwenjun added this to the 3.2.0 milestone Apr 4, 2023
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_alertServerSupportCluster branch 5 times, most recently from 41c2ab6 to dfbecc8 Compare April 4, 2023 09:25
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2023

Codecov Report

Merging #13865 (d6ee822) into dev (6483395) will decrease coverage by 0.17%.
The diff coverage is 9.21%.

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

@@             Coverage Diff              @@
##                dev   #13865      +/-   ##
============================================
- Coverage     39.10%   38.93%   -0.17%     
+ Complexity     4460     4450      -10     
============================================
  Files          1155     1157       +2     
  Lines         42270    42340      +70     
  Branches       4760     4771      +11     
============================================
- Hits          16528    16486      -42     
- Misses        23929    24036     +107     
- Partials       1813     1818       +5     
Impacted Files Coverage Δ
...org/apache/dolphinscheduler/alert/AlertServer.java 0.00% <0.00%> (ø)
...hinscheduler/alert/metrics/AlertServerMetrics.java 0.00% <ø> (ø)
...phinscheduler/alert/plugin/AlertPluginManager.java 2.43% <0.00%> (ø)
...inscheduler/alert/registry/AlertHeartbeatTask.java 0.00% <0.00%> (ø)
...nscheduler/alert/registry/AlertRegistryClient.java 0.00% <0.00%> (ø)
...phinscheduler/alert/rpc/AlertRequestProcessor.java 0.00% <0.00%> (ø)
...che/dolphinscheduler/alert/rpc/AlertRpcServer.java 0.00% <0.00%> (ø)
...er/api/service/impl/MetricsCleanUpServiceImpl.java 14.28% <0.00%> (ø)
...e/dolphinscheduler/common/constants/Constants.java 75.00% <ø> (ø)
...lphinscheduler/common/model/BaseHeartBeatTask.java 0.00% <0.00%> (ø)
... and 21 more

... and 3 files with indirect coverage changes

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

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_alertServerSupportCluster branch from dfbecc8 to 483af6d Compare April 4, 2023 09:51
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_alertServerSupportCluster branch from 483af6d to 3f538d1 Compare April 4, 2023 10:05
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

The idea looks good!

Comment on lines -174 to -175
alert-listen-host: localhost
alert-listen-port: 50052
Copy link
Member

Choose a reason for hiding this comment

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

Can you review these changed configurations in the Helm Chart and see whether we have these there, if so remove them as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I find these exist in kubernetes and terraform, and remove these, please help to check again.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 4, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 14 Code Smells

28.4% 28.4% Coverage
0.0% 0.0% Duplication

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_alertServerSupportCluster branch from 89e97da to 9e50103 Compare April 4, 2023 15:56
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_alertServerSupportCluster branch from 9e50103 to dc4f362 Compare April 4, 2023 15:57
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 4, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 14 Code Smells

28.4% 28.4% Coverage
0.0% 0.0% Duplication

@ruanwenjun ruanwenjun requested a review from kezhenxu94 April 5, 2023 08:52
@ruanwenjun ruanwenjun merged commit 41a8ba9 into apache:dev Apr 5, 2023
@ruanwenjun ruanwenjun deleted the dev_wenjun_alertServerSupportCluster branch April 5, 2023 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.2.0 for 3.2.0 version backend feature new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants