[GitHub] [pulsar] heesung-sn commented on a diff in pull request #19154: [improve][broker] PIP-192 Moved the common broker load data feature(weightedMaxEMA) to BrokerLoadData

[GitBox]

heesung-sn commented on code in PR #19154:
URL: https://github.com/apache/pulsar/pull/19154#discussion_r1072701722


##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/data/BrokerLoadData.java:
##
@@ -95,17 +124,35 @@ private void updateSystemResourceUsage(final ResourceUsage 
cpu, final ResourceUs
 this.bandwidthOut = bandwidthOut;
 }
 
-public double getMaxResourceUsage() {
-return LocalBrokerData.max(cpu.percentUsage(), 
directMemory.percentUsage(), bandwidthIn.percentUsage(),
+private void updateFeatures(ServiceConfiguration conf) {
+updateMaxResourceUsage();
+updateWeightedMaxEMA(conf);
+}
+
+private void updateMaxResourceUsage() {
+maxResourceUsage = LocalBrokerData.max(cpu.percentUsage(), 
directMemory.percentUsage(),
+bandwidthIn.percentUsage(),
 bandwidthOut.percentUsage()) / 100;
 }
 
-public double getMaxResourceUsageWithWeight(final double cpuWeight, final 
double memoryWeight,
+private double getMaxResourceUsageWithWeight(final double cpuWeight, final 
double memoryWeight,
 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) {

Review Comment:
   It is an exponential moving average. Updated the comment for the 
weightedMaxEMA variable.



##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/strategy/LeastResourceUsageWithWeight.java:
##
@@ -80,40 +74,6 @@ private double getMaxResourceUsageWithWeight(final String 
broker, final BrokerLo
 return maxUsageWithWeight;
 }
 
-/**
- * Update and get the max resource usage with weight of broker according 
to the service configuration.
- *
- * @param broker The broker name.
- * @param brokerData The broker load data.
- * @param conf   The service configuration.
- * @param debugMode  The debug mode to print computed load states and 
decision information.
- * @return the max resource usage with weight of broker
- */
-private double updateAndGetMaxResourceUsageWithWeight(String broker, 
BrokerLoadData brokerData,
-  ServiceConfiguration 
conf, boolean debugMode) {
-final double historyPercentage = 
conf.getLoadBalancerHistoryResourcePercentage();
-Double historyUsage = brokerAvgResourceUsageWithWeight.get(broker);
-double resourceUsage = brokerData.getMaxResourceUsageWithWeight(
-conf.getLoadBalancerCPUResourceWeight(),
-conf.getLoadBalancerMemoryResourceWeight(),
-conf.getLoadBalancerDirectMemoryResourceWeight(),
-conf.getLoadBalancerBandwithInResourceWeight(),
-conf.getLoadBalancerBandwithOutResourceWeight());
-historyUsage = historyUsage == null
-? resourceUsage : historyUsage * historyPercentage + (1 - 
historyPercentage) * resourceUsage;
-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());
-}

Review 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));
   }
   
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [pulsar] heesung-sn commented on a diff in pull request #19154: [improve][broker] PIP-192 Moved the common broker load data feature(weightedMaxEMA) to BrokerLoadData

[GitBox]

heesung-sn commented on code in PR #19154:
URL: https://github.com/apache/pulsar/pull/19154#discussion_r1065026187


##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/strategy/LeastResourceUsageWithWeight.java:
##
@@ -43,21 +41,17 @@ public class LeastResourceUsageWithWeight implements 
BrokerSelectionStrategy {
 // Maintain this list to reduce object creation.
 private final ArrayList bestBrokers;
 private final Set noLoadDataBrokers;
-private final Map brokerAvgResourceUsageWithWeight;
 
 public LeastResourceUsageWithWeight() {
 this.bestBrokers = new ArrayList<>();
-this.brokerAvgResourceUsageWithWeight = new HashMap<>();
 this.noLoadDataBrokers = new HashSet<>();
 }
 
 // A broker's max resource usage with weight using its historical load and 
short-term load data with weight.
 private double getMaxResourceUsageWithWeight(final String broker, final 
BrokerLoadData brokerLoadData,
  final ServiceConfiguration 
conf, boolean debugMode) {
 final double overloadThreshold = 
conf.getLoadBalancerBrokerOverloadedThresholdPercentage() / 100.0;
-final double maxUsageWithWeight =
-updateAndGetMaxResourceUsageWithWeight(broker, brokerLoadData, 
conf, debugMode);
-
+final var maxUsageWithWeight = brokerLoadData.getWeightedMaxEMA();

Review 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.`
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org