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.

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

[Fix] Split cpuUsage into systemCpuUsage and jvmCpuUsage #15803

Merged
merged 1 commit into from
Apr 9, 2024
Merged

[Fix] Split cpuUsage into systemCpuUsage and jvmCpuUsage #15803

merged 1 commit into from
Apr 9, 2024

Conversation

ruanwenjun
Copy link
Member

@ruanwenjun ruanwenjun commented Apr 7, 2024 •
edited
Loading

Purpose of the pull request

fix #15579

Brief change log

Split maxCpuUsagePercentageThresholds into maxSystemCpuUsagePercentageThresholds and maxJvmCpuUsagePercentageThresholds, to avoid judging overload by CPU might confusion.

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 Split cpuUsage to systemCpuUsage and jvmCpuUsage [Fix] Split cpuUsage to systemCpuUsage and jvmCpuUsage Apr 7, 2024
@ruanwenjun ruanwenjun mentioned this pull request Apr 7, 2024
Closed
@ruanwenjun ruanwenjun self-assigned this Apr 7, 2024
@ruanwenjun ruanwenjun added the bug Something isn't working label Apr 7, 2024
@ruanwenjun ruanwenjun changed the title [Fix] Split cpuUsage to systemCpuUsage and jvmCpuUsage [Fix] Split cpuUsage into systemCpuUsage and jvmCpuUsage Apr 7, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 39.17%. Comparing base (9599b5a) to head (aec472b).

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

Files Patch % Lines
...inscheduler/alert/registry/AlertHeartbeatTask.java 0.00% 2 Missing ⚠️
...eduler/server/master/task/MasterHeartBeatTask.java 0.00% 2 Missing ⚠️
...e/dolphinscheduler/server/master/MasterServer.java 0.00% 1 Missing ⚠️
...ler/server/master/metrics/MasterServerMetrics.java 0.00% 1 Missing ⚠️
...e/dolphinscheduler/server/worker/WorkerServer.java 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##                dev   #15803      +/-   ##
============================================
- Coverage     39.20%   39.17%   -0.04%     
+ Complexity     4909     4888      -21     
============================================
  Files          1326     1323       -3     
  Lines         45217    45184      -33     
  Branches       4818     4810       -8     
============================================
- Hits          17729    17702      -27     
+ Misses        25617    25603      -14     
- Partials       1871     1879       +8

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

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixCPUMetrics branch 4 times, most recently from 09951e1 to 9406a6b Compare April 7, 2024 04:47
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 7, 2024
View reviewed changes
...inscheduler-common/src/main/java/org/apache/dolphinscheduler/common/model/BaseHeartBeat.java Fixed Show fixed Hide fixed
...inscheduler-common/src/main/java/org/apache/dolphinscheduler/common/model/BaseHeartBeat.java Fixed Show fixed Hide fixed
...inscheduler-common/src/main/java/org/apache/dolphinscheduler/common/model/BaseHeartBeat.java
protected double jvmMemoryUsage;
protected double memoryUsage;
protected double diskUsage;
protected ServerStatus serverStatus;

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
HeartBeat.getServerStatus
Loading
; it is advisable to add an Override annotation.
...scheduler-common/src/main/java/org/apache/dolphinscheduler/common/model/MasterHeartBeat.java Fixed Show fixed Hide fixed
...scheduler-common/src/main/java/org/apache/dolphinscheduler/common/model/WorkerHeartBeat.java Fixed Show fixed Hide fixed
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixCPUMetrics branch 5 times, most recently from 506c918 to b952ea4 Compare April 7, 2024 06:03
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 7, 2024
View reviewed changes
...scheduler-common/src/main/java/org/apache/dolphinscheduler/common/model/WorkerHeartBeat.java

@Data
@Builder
@EqualsAndHashCode(callSuper = true)

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
BaseHeartBeat.canEqual
Loading
; it is advisable to add an Override annotation.
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixCPUMetrics branch 2 times, most recently from fb2d3ac to 8e8ed13 Compare April 8, 2024 13:58
@ruanwenjun ruanwenjun mentioned this pull request Apr 8, 2024
Closed
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixCPUMetrics branch from 8e8ed13 to 22bd4c1 Compare April 9, 2024 01:50
rickchengx
rickchengx reviewed Apr 9, 2024
View reviewed changes
...-meter/src/main/java/org/apache/dolphinscheduler/meter/metrics/BaseServerLoadProtection.java Outdated
systemMetrics.getJvmCpuUsagePercentage(), maxJvmCpuUsagePercentageThresholds);
return true;
}
if (systemMetrics.getJvmMemoryUsedPercentage() > maxJvmMemoryUsagePercentageThresholds) {
Copy link
Contributor

@rickchengx rickchengx Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if judging a jvm's memory usage is necessary. I think the heap memory of JVM will always increase over time. After reaching certain conditions, minor/major GC will be triggered and the memory usage will decrease.

Therefore, I think that the temporary increase in jvm memory utilization is not abnormal. But due to memory leaks and other reasons, it is indeed an anomaly that the JVM's memory utilization remains high for a long time.

Others LGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I have removed this setting, PTAL

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixCPUMetrics branch from 22bd4c1 to 34a56cd Compare April 9, 2024 03:58
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixCPUMetrics branch from 34a56cd to 705b1fa Compare April 9, 2024 04:00
@ruanwenjun ruanwenjun requested a review from rickchengx April 9, 2024 04:00
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Apr 9, 2024

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

rickchengx
rickchengx approved these changes Apr 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

@ruanwenjun ruanwenjun merged commit 66df5d4 into apache:dev Apr 9, 2024
59 of 60 checks passed
@ruanwenjun ruanwenjun deleted the dev_wenjun_fixCPUMetrics branch April 9, 2024 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickchengx rickchengx rickchengx approved these changes

@EricGao888 EricGao888 Awaiting requested review from EricGao888 EricGao888 is a code owner

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

@SbloodyS SbloodyS Awaiting requested review from SbloodyS SbloodyS is a code owner

Assignees

@ruanwenjun ruanwenjun

Labels
backend bug Something isn't working document ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Fix][e2e] Fix e2e failed due to server overload.
3 participants
@ruanwenjun @codecov-commenter @rickchengx