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

[#796][0.7] bug: Fix the issues of MetricReporter #821

Merged
merged 4 commits into from
Apr 13, 2023

Conversation

xianjingfeng
Copy link
Member

What changes were proposed in this pull request?

  1. Support custom config keys defined in plugins
  2. Refactor the logic for load config file
  3. Fix some issues of metricReporter.

Why are the changes needed?

Metric reporter is unusable.
Fix: #796

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UT and Manual testing

### What changes were proposed in this pull request?
Support custom config keys defined in plugins
Refactor the logic for load config file
Fix some issues of metricReporter.
### Why are the changes needed?
Metric reporter is unusable.
Fix: apache#796

### Does this PR introduce any user-facing change?
No.

### How was this patch tested?
UT and Manual testing
# Conflicts:
#	common/src/main/java/org/apache/uniffle/common/config/RssBaseConf.java
#	server/src/test/java/org/apache/uniffle/server/ShuffleServerConfTest.java
@xianjingfeng xianjingfeng requested review from jerqi and zuston April 13, 2023 02:07
@jerqi jerqi changed the title [#796] bug:fix the issues of MetricReporter(for 0.7) [#796][0.7] bug:fix the issues of MetricReporter Apr 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2023

Codecov Report

Merging #821 (20a0a9a) into branch-0.7 (6857ea5) will decrease coverage by 0.12%.
The diff coverage is 13.04%.

@@               Coverage Diff                @@
##             branch-0.7     #821      +/-   ##
================================================
- Coverage         60.89%   60.78%   -0.12%     
+ Complexity         1800     1792       -8     
================================================
  Files               214      214              
  Lines             12386    12374      -12     
  Branches           1042     1045       +3     
================================================
- Hits               7543     7521      -22     
- Misses             4440     4449       +9     
- Partials            403      404       +1     
Impacted Files Coverage Δ
.../org/apache/uniffle/common/config/RssBaseConf.java 96.34% <0.00%> (+2.29%) ⬆️
...java/org/apache/uniffle/common/config/RssConf.java 29.93% <0.00%> (-2.48%) ⬇️
...rometheus/PrometheusPushGatewayMetricReporter.java 74.46% <0.00%> (ø)
.../apache/uniffle/coordinator/CoordinatorServer.java 56.45% <0.00%> (-0.93%) ⬇️
.../java/org/apache/uniffle/server/ShuffleServer.java 63.51% <0.00%> (-0.88%) ⬇️
.../uniffle/common/metrics/MetricReporterFactory.java 57.14% <100.00%> (ø)
...rg/apache/uniffle/coordinator/CoordinatorConf.java 98.70% <100.00%> (+1.11%) ⬆️
...a/org/apache/uniffle/server/ShuffleServerConf.java 99.28% <100.00%> (-0.04%) ⬇️

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

@xianjingfeng
Copy link
Member Author

It seems that this PR is merged to master in CI enviroment. But the first time is ok.
https://github.com/apache/incubator-uniffle/actions/runs/4684746312/jobs/8301194063

Do you know why? @kaijchen @jerqi

@jerqi
Copy link
Contributor

jerqi commented Apr 13, 2023

It seems that this PR is merged to master in CI enviroment. But the first time is ok. https://github.com/apache/incubator-uniffle/actions/runs/4684746312/jobs/8301194063

Do you know why? @kaijchen @jerqi

I cherry-pick a wrong commit. I have reverted it. it's strange that it influence your pr. You could rerun your pr.

@kaijchen
Copy link
Contributor

You could rerun your pr.

Please rebase or merge branch-0.7.

@jerqi
Copy link
Contributor

jerqi commented Apr 13, 2023

You could rerun your pr.

Please rebase or merge branch-0.7.

企业微信截图_90010f5d-d8b3-4afd-9ad7-45b8ba63e889

But his commit history don't have my mistake commit. It's strange. But the error seems that it caused by my mistake commit.

@xianjingfeng
Copy link
Member Author

It is ok now, just reopen it.

Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @xianjingfeng

@jerqi jerqi changed the title [#796][0.7] bug:fix the issues of MetricReporter [#796][0.7] bug: Fix the issues of MetricReporter Apr 13, 2023
@xianjingfeng xianjingfeng merged commit bcb591e into apache:branch-0.7 Apr 13, 2023
@xianjingfeng xianjingfeng deleted the 796_for_0.7 branch April 13, 2023 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants