[#796] bug:fix the issues of MetricReporter #797
Conversation
# common/src/main/java/org/apache/uniffle/common/config/RssBaseConf.java # server/src/main/java/org/apache/uniffle/server/ShuffleServer.java # server/src/test/java/org/apache/uniffle/server/ShuffleServerConfTest.java
Codecov Report
@@ Coverage Diff @@
## master #797 +/- ##
============================================
+ Coverage 58.06% 59.41% +1.34%
+ Complexity 2072 2065 -7
============================================
Files 304 290 -14
Lines 14810 12831 -1979
Branches 1212 1214 +2
============================================
- Hits 8600 7624 -976
+ Misses 5720 4778 -942
+ Partials 490 429 -61
... and 14 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This reverts commit a3560d3.
|
You have become the committer. You have the authority to rerun the failure tests. You don't need to reopen the pr. |
common/src/main/java/org/apache/uniffle/common/metrics/MetricReporterFactory.java
Show resolved
Hide resolved
| @@ -28,3 +28,4 @@ rss.coordinator.access.candidates.updateIntervalSec 1 | |||
| rss.coordinator.access.loadChecker.serverNum.threshold 2 | |||
| rss.coordinator.access.loadChecker.memory.percentage 20.0 | |||
| rss.coordinator.dynamicClientConf.updateIntervalSec 1 | |||
| plugin.custom.key v1 | |||
There was a problem hiding this comment.
Could we have a better format? We will have many plugins. What's type of plugin? The plugin is used for shuffle server , Coordinator or both. I can't get enough information.
There was a problem hiding this comment.
Could we have a better format? We will have many plugins. What's type of plugin? The plugin is used for shuffle server , Coordinator or both. I can't get enough information.
It is just for UT and it is not actually used.
I just merge master into this PR, and then i found some changes in #792 appear in the |
common/src/main/java/org/apache/uniffle/common/config/RssConf.java
Outdated
Show resolved
Hide resolved
common/src/main/java/org/apache/uniffle/common/config/RssConf.java
Outdated
Show resolved
Hide resolved
| @@ -105,6 +105,7 @@ PrometheusPushGatewayMetricReporter is one of the built-in metrics reporter, whi | |||
|
|
|||
| |Property Name|Default| Description | | |||
| |---|---|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | |||
| |rss.metrics.reporter.class|org.apache.uniffle.common.metrics.<br/>prometheus.PrometheusPushGatewayMetricReporter|The class of metrics reporter.| | |||
There was a problem hiding this comment.
nit: do you think is it necessary to include <br/> here?
There was a problem hiding this comment.
Because the class name is too long, the length occupied by this column is too long, and this table needs to be scrolled, making it inconvenient to view.
|
Merged, thanks @advancedxy @jerqi . |
It's OK for me. Let me cherry pick this. |
|
@xianjingfeng There is conflict with branch 0.7. We can't cherry-pick directly. Could you raise a new pr for branch 0.7? |
### 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
|
What changes were proposed in this pull request?
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