[improve][broker] PIP-192 Moved the common broker load data feature(weightedMaxEMA) to BrokerLoadData #19154
Conversation
…eightedMaxEMA) to BrokerLoadData
| final double maxUsageWithWeight = | ||
| updateAndGetMaxResourceUsageWithWeight(broker, brokerLoadData, conf, debugMode); | ||
|
|
||
| final var maxUsageWithWeight = brokerLoadData.getWeightedMaxEMA(); |
There was a problem hiding this comment.
The weightedMaxEMA can be null. Should we handle the null case here?
There was a problem hiding this comment.
Thanks for this comment.
I changed this weightedMaxEMA to 'double` and set the default value to 1.0.
Ideally, the downstream logics using this BrokerLoadData should not worry if it is updated or not.
Let's ensure we publish BrokerLoadData to the service unit channel only when it has been updated at least once.
Also, accordingly, I cleaned up BrokerLoadData.
heesung-sn
force-pushed
the
pip-192-feature-store
branch
from
9528960
to
0e6da72
Compare
heesung-sn
force-pushed
the
pip-192-feature-store
branch
from
0e6da72
to
8145f59
Compare
Demogorgon314
requested review from
BewareMyPower,
codelipenghui,
congbobo184,
eolivelli,
mattisonchao and
Technoboy-
| final double directMemoryWeight, final double bandwidthInWeight, | ||
| final double bandwidthOutWeight) { | ||
| return LocalBrokerData.max(cpu.percentUsage() * cpuWeight, memory.percentUsage() * memoryWeight, | ||
| directMemory.percentUsage() * directMemoryWeight, bandwidthIn.percentUsage() * bandwidthInWeight, | ||
| bandwidthOut.percentUsage() * bandwidthOutWeight) / 100; | ||
| } | ||
|
|
||
|
|
||
| private void updateWeightedMaxEMA(ServiceConfiguration conf) { |
There was a problem hiding this comment.
What does EMA mean? Is it better to add a description for "EMA"?
There was a problem hiding this comment.
It is an exponential moving average. Updated the comment for the weightedMaxEMA variable.
| if (debugMode) { | ||
| log.info( | ||
| "Broker {} get max resource usage with weight: {}, history resource percentage: {}%, CPU weight: " | ||
| + "{}, MEMORY weight: {}, DIRECT MEMORY weight: {}, BANDWIDTH IN weight: {}, BANDWIDTH " | ||
| + "OUT weight: {} ", | ||
| broker, historyUsage, historyPercentage, conf.getLoadBalancerCPUResourceWeight(), | ||
| conf.getLoadBalancerMemoryResourceWeight(), conf.getLoadBalancerDirectMemoryResourceWeight(), | ||
| conf.getLoadBalancerBandwithInResourceWeight(), | ||
| conf.getLoadBalancerBandwithOutResourceWeight()); | ||
| } |
There was a problem hiding this comment.
Should the debug log be removed?
There was a problem hiding this comment.
No. I think we can keep this.
Added BrokerLoadData.toString() and added the debug log.
else if (debugMode) {
log.info("Broker {} load data:{{}}", broker, brokerLoadData.toString(conf));
}
Codecov Report
@@ Coverage Diff @@
## master #19154 +/- ##
============================================
- Coverage 47.45% 41.04% -6.42%
- Complexity 10760 15887 +5127
============================================
Files 713 1625 +912
Lines 69672 132324 +62652
Branches 7482 14564 +7082
============================================
+ Hits 33063 54312 +21249
- Misses 32895 72092 +39197
- Partials 3714 5920 +2206
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Master Issue: #16691
Motivation
We will start raising PRs to implement PIP-192, #16691
Modifications
This PR moved the common broker load data feature(
weightedMaxEMA) toBrokerLoadData.As
weightedMaxEMAis available in theBrokerLoadDataby this change, this PRremoved
brokerAvgResourceUsageWithWeightin LeastResourceUsageWithWeight -- we don't need to separately compute and storeweightedMaxEMAinLeastResourceUsageWithWeight and TransferShedder.refactored BrokerLoadData to precompute the load data features.
updated the unit tests.
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeWe will have separate PRs to update the Doc later.
Matching PR in forked repository
PR in forked repository: heesung-sn#20