You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/10/23 10:32:27 UTC
[GitHub] [pulsar] lordcheng10 opened a new pull request #12471: Optimize ThresholdShedder strategy
lordcheng10 opened a new pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471
### Motivation
Consider a large cluster with multiple nodes. If most of the nodes are relatively balanced, but only a few nodes have extremely low load, then according to the current logic, it may not be possible to trigger the balancing action, eg:
broke1 brokerAvgResourceUsage 80
......
broker100 brokerAvgResourceUsage 80
broker101 brokerAvgResourceUsage 10
The calculated avgUsage=(80*100+10)/101=79 at this time, if the threshold is set to the default value of 10, then any broker will satisfy currentUsage <avgUsage + threshold, so that the balancing operation will not be triggered, so some load Low machines cannot be fully utilized.
### Modifications
Here I define the equilibrium state as:
avgUsage-threshold< currentUsage <avgUsage + threshold
In this way, some nodes with extremely low load will not appear.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on a change in pull request #12471: Optimize ThresholdShedder strategy
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#discussion_r735197533
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java
##########
@@ -78,7 +78,7 @@
final LocalBrokerData localData = brokerData.getLocalData();
final double currentUsage = brokerAvgResourceUsage.getOrDefault(broker, 0.0);
- if (currentUsage < avgUsage + threshold) {
+ if (currentUsage > avgUsage - threshold && currentUsage < avgUsage + threshold) {
Review comment:
The following operation only works for overload broker node, which unLoad bundles from the broker. If the broker with low load, and applied the above check, it will also be filtered by
```Java
if (minimumThroughputToOffload < minThroughputThreshold) {
if (log.isDebugEnabled()) {
log.info("[{}] broker is planning to shed throughput {} MByte/s less than "
+ "minimumThroughputThreshold {} MByte/s, skipping bundle unload.",
broker, minimumThroughputToOffload / MB, minThroughputThreshold / MB);
}
return;
}
```
If we want to improve the load of low load broker, we can't operate the broker directly, it can only depend on other broker unload bundle and the leader select the low load broker and onload the bundles on it.
We should consider the loadbalance mechanism to address the issue.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-954029746
I made some changes. PTAL，thanks! @hangc0276
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on a change in pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#discussion_r742702270
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java
##########
@@ -74,19 +77,56 @@
return selectedBundlesCache;
}
+ AtomicBoolean isBalanced = new AtomicBoolean(true);
+ Map<String, BrokerData> belowAvgUsageBrokers = new HashMap<>();
+ Map<String, BrokerData> overAvgUsageBrokers = new HashMap<>();
+ AtomicDouble sumOfAcceptableTrafficFromBelowAvgUsageBrokers = new AtomicDouble(0);
+ AtomicDouble totalTrafficOfOverAvgUsageBrokers = new AtomicDouble(0);
+
+ // 1. Divided into two categories：① (currentUsage > avgUsage); ②(currentUsage < avgUsage);
loadData.getBrokerData().forEach((broker, brokerData) -> {
+ final LocalBrokerData localData = brokerData.getLocalData();
+ double brokerCurrentThroughput = localData.getMsgThroughputIn() + localData.getMsgThroughputOut();
+ final double currentUsage = brokerAvgResourceUsage.getOrDefault(broker, 0.0);
+
+ if (currentUsage > avgUsage - threshold && currentUsage < avgUsage + threshold) {
+ isBalanced.set(false);
+ return;
+ }
+ if (currentUsage < avgUsage) {
+ double percentOfTrafficToAccept = avgUsage - currentUsage;
+ double trafficToAccept = percentOfTrafficToAccept * brokerCurrentThroughput;
+ //2. Calculate the sum of acceptable traffic in belowAvgUsageBrokers
+ sumOfAcceptableTrafficFromBelowAvgUsageBrokers.addAndGet(trafficToAccept);
+ belowAvgUsageBrokers.put(broker, brokerData);
+ }
+ if (currentUsage > avgUsage) {
+ //3. Calculate the total traffic in overAvgUsageBrokers
+ totalTrafficOfOverAvgUsageBrokers.addAndGet(brokerCurrentThroughput);
+ overAvgUsageBrokers.put(broker, brokerData);
+ }
+ });
+
+ if (isBalanced.get()) {
+ return selectedBundlesCache;
+ }
+
+ //4. Calculate the percentage of traffic to be migrated by each broker in overAvgUsageBrokers;
+ double percentOfTrafficToOffload = ADDITIONAL_THRESHOLD_PERCENT_MARGIN
Review comment:
>> Why do we calculate this as a generic average for all brokers?
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-973657415
> the current implementation is by design to fully utilize the broker. so, if needed we need to introduce new policy which can be configurable.
Okay, I will rewrite an equilibrium strategy。
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #12471: Optimize ThresholdShedder strategy
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-950131799
@lordcheng10:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-973238703
also , don't change the existing implementation because there are two different approaches for load balancer a. utilize brokers fully b. distribute load uniformly across brokers. so, we should not change existing behavior and this behavior should be configurable.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-973235424
this solution would not work in heterogeneous hardware and not correct to make it default behavior. I have done some work to address this issue and need to complete the PR.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-973657177
> @lordcheng10 - instead of writing a diagram, I think it would be more productive to write tests that show how it will work and will also serve to verify your algorithm works the way your say it will.
>
> I am still of the opinion that this is a new algorithm. That does not mean you shouldn't implement it, it just means that updating the `ThresholdShedder` might be the wrong direction. I am interested to know what others think, too.
Okay, I will rewrite an equilibrium strategy。
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-956014078
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-955736427
> I wonder if it would make more sense to implement a new `LoadSheddingStrategy` to achieve your goals instead of updating the `ThesholdShedder` class. This class is only meant to unload bundles from a broker with above average (plus some threshold) usage.
>
> Based on your PR's description, you'd like the `ThresholdShedder` algorithm to take under utilized brokers into special consideration. As @hangc0276 pointed out, it is not straightforward to consider these brokers because the only mechanism we have available in this class is to "unload" a bundle from a broker. Your proposed changes require the unloading of bundles from brokers that are not above the average (plus some threshold) usage, which seems like a new strategy to me.
>
> Note that because we're using a simple average, clusters with many brokers might have a few under utilized brokers without any one broker being too much above the average utilization. An alternative strategy could give underutilized brokers more weight.
>
> > The calculated avgUsage=(80*100+10)/101=79 at this time, if the threshold is set to the default value of 10, then any broker will satisfy currentUsage <avgUsage + threshold, so that the balancing operation will not be triggered, so some load Low machines cannot be fully utilized.
>
> Based on your example, an alternative solution is to configure a smaller threshold. Decreasing the threshold will lead to better distribution, but the trade off is that it will also lead to more frequent unloading, which has costs too.
>
> If we do decide to update this class's strategy, we will need to document the new details in the Javadoc as well as add tests to ensure that the algorithm works correctly.
Modified balance strategy is not to unload from the Brokers below the average value, but to calculate the total load that the Brokers below the average value can receive, so as to calculate the ratio of unloading from the Brokers above the average value.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-954029746
In response to the problem you said, I fixed it.
PTAL，thanks! @hangc0276
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-973218773
@lordcheng10 - instead of writing a diagram, I think it would be more productive to write tests that show how it will work and will also serve to verify your algorithm works the way your say it will.
I am still of the opinion that this is a new algorithm. That does not mean you shouldn't implement it, it just means that updating the `ThresholdShedder` might be the wrong direction. I am interested to know what others think, too.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-973196953
In order to facilitate understanding, I have shown the code ideas in the form of diagrams. PTAL,thanks! @hangc0276 @michaeljmarshall
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12471: Optimize ThresholdShedder strategy
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-950181636
/pulsarbot run-failure-checks
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-973657177
> @lordcheng10 - instead of writing a diagram, I think it would be more productive to write tests that show how it will work and will also serve to verify your algorithm works the way your say it will.
>
> I am still of the opinion that this is a new algorithm. That does not mean you shouldn't implement it, it just means that updating the `ThresholdShedder` might be the wrong direction. I am interested to know what others think, too.
You are right, I will rewrite a balanced strategy class！
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-973657415
> the current implementation is by design to fully utilize the broker. so, if needed we need to introduce new policy which can be configurable.
You are right, I will rewrite a balanced strategy class
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12471: Optimize ThresholdShedder strategy
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-950256827
/pulsarbot run-failure-checks
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12471: Optimize ThresholdShedder strategy
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-950154622
/pulsarbot run-failure-checks
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-974601839
@lordcheng10 can you please check if this PR addresses your concern https://github.com/apache/pulsar/pull/12902
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on a change in pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#discussion_r742702270
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java
##########
@@ -74,19 +77,56 @@
return selectedBundlesCache;
}
+ AtomicBoolean isBalanced = new AtomicBoolean(true);
+ Map<String, BrokerData> belowAvgUsageBrokers = new HashMap<>();
+ Map<String, BrokerData> overAvgUsageBrokers = new HashMap<>();
+ AtomicDouble sumOfAcceptableTrafficFromBelowAvgUsageBrokers = new AtomicDouble(0);
+ AtomicDouble totalTrafficOfOverAvgUsageBrokers = new AtomicDouble(0);
+
+ // 1. Divided into two categories：① (currentUsage > avgUsage); ②(currentUsage < avgUsage);
loadData.getBrokerData().forEach((broker, brokerData) -> {
+ final LocalBrokerData localData = brokerData.getLocalData();
+ double brokerCurrentThroughput = localData.getMsgThroughputIn() + localData.getMsgThroughputOut();
+ final double currentUsage = brokerAvgResourceUsage.getOrDefault(broker, 0.0);
+
+ if (currentUsage > avgUsage - threshold && currentUsage < avgUsage + threshold) {
+ isBalanced.set(false);
+ return;
+ }
+ if (currentUsage < avgUsage) {
+ double percentOfTrafficToAccept = avgUsage - currentUsage;
+ double trafficToAccept = percentOfTrafficToAccept * brokerCurrentThroughput;
+ //2. Calculate the sum of acceptable traffic in belowAvgUsageBrokers
+ sumOfAcceptableTrafficFromBelowAvgUsageBrokers.addAndGet(trafficToAccept);
+ belowAvgUsageBrokers.put(broker, brokerData);
+ }
+ if (currentUsage > avgUsage) {
+ //3. Calculate the total traffic in overAvgUsageBrokers
+ totalTrafficOfOverAvgUsageBrokers.addAndGet(brokerCurrentThroughput);
+ overAvgUsageBrokers.put(broker, brokerData);
+ }
+ });
+
+ if (isBalanced.get()) {
+ return selectedBundlesCache;
+ }
+
+ //4. Calculate the percentage of traffic to be migrated by each broker in overAvgUsageBrokers;
+ double percentOfTrafficToOffload = ADDITIONAL_THRESHOLD_PERCENT_MARGIN
Review comment:
>> Why do we calculate this as a generic average for all brokers?
In the example I mentioned above, the load utilization rate of all brokers is lower than the average utilization rate. At this time, according to the original calculation method, there is no way to calculate percentoftraffictooffload.
With regard to this new change, I need to explain the following points:
1. First, I define the equilibrium state as:
avgUsage - threshold < currentUsage < avgUsage + threshold；
2. The sum of the traffic that can be received by all nodes lower than avgusage is the total traffic that we should unload from nodes higher than avgusage. Therefore, we only need to calculate the sum of the total traffic of all brokers higher than avgusage over brokertrafficsum, Then, sumbelowoavgtraffic / overbrokertrafficsum can get the information from each broker that exceeds avgusage
@michaeljmarshall
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on a change in pull request #12471: Optimize ThresholdShedder strategy
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#discussion_r736079979
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java
##########
@@ -78,7 +78,7 @@
final LocalBrokerData localData = brokerData.getLocalData();
final double currentUsage = brokerAvgResourceUsage.getOrDefault(broker, 0.0);
- if (currentUsage < avgUsage + threshold) {
+ if (currentUsage > avgUsage - threshold && currentUsage < avgUsage + threshold) {
Review comment:
You are right， I will fix it
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-954326378
/pulsarbot run-failure-checks
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-1072917316
The pr had no activity for 30 days, mark with Stale label.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-1036507027
Removing the `release/2.8.3` and `release/2.9.3` labels as this change will target master and probably won't be cherry picked back to stable branches because it is an optimization.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-955736427
> I wonder if it would make more sense to implement a new `LoadSheddingStrategy` to achieve your goals instead of updating the `ThesholdShedder` class. This class is only meant to unload bundles from a broker with above average (plus some threshold) usage.
>
> Based on your PR's description, you'd like the `ThresholdShedder` algorithm to take under utilized brokers into special consideration. As @hangc0276 pointed out, it is not straightforward to consider these brokers because the only mechanism we have available in this class is to "unload" a bundle from a broker. Your proposed changes require the unloading of bundles from brokers that are not above the average (plus some threshold) usage, which seems like a new strategy to me.
>
> Note that because we're using a simple average, clusters with many brokers might have a few under utilized brokers without any one broker being too much above the average utilization. An alternative strategy could give underutilized brokers more weight.
>
> > The calculated avgUsage=(80*100+10)/101=79 at this time, if the threshold is set to the default value of 10, then any broker will satisfy currentUsage <avgUsage + threshold, so that the balancing operation will not be triggered, so some load Low machines cannot be fully utilized.
>
> Based on your example, an alternative solution is to configure a smaller threshold. Decreasing the threshold will lead to better distribution, but the trade off is that it will also lead to more frequent unloading, which has costs too.
>
> If we do decide to update this class's strategy, we will need to document the new details in the Javadoc as well as add tests to ensure that the algorithm works correctly.
My modified balance strategy is not to unload from the Brokers below the average value, but to calculate the total load that the Brokers below the average value can receive, so as to calculate the ratio of unloading from the Brokers above the average value.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#discussion_r738693507
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java
##########
@@ -74,19 +77,56 @@
return selectedBundlesCache;
}
+ AtomicBoolean isBalanced = new AtomicBoolean(true);
+ Map<String, BrokerData> belowAvgUsageBrokers = new HashMap<>();
+ Map<String, BrokerData> overAvgUsageBrokers = new HashMap<>();
+ AtomicDouble sumOfAcceptableTrafficFromBelowAvgUsageBrokers = new AtomicDouble(0);
+ AtomicDouble totalTrafficOfOverAvgUsageBrokers = new AtomicDouble(0);
+
+ // 1. Divided into two categories：① (currentUsage > avgUsage); ②(currentUsage < avgUsage);
loadData.getBrokerData().forEach((broker, brokerData) -> {
+ final LocalBrokerData localData = brokerData.getLocalData();
+ double brokerCurrentThroughput = localData.getMsgThroughputIn() + localData.getMsgThroughputOut();
+ final double currentUsage = brokerAvgResourceUsage.getOrDefault(broker, 0.0);
+
+ if (currentUsage > avgUsage - threshold && currentUsage < avgUsage + threshold) {
+ isBalanced.set(false);
+ return;
+ }
+ if (currentUsage < avgUsage) {
+ double percentOfTrafficToAccept = avgUsage - currentUsage;
+ double trafficToAccept = percentOfTrafficToAccept * brokerCurrentThroughput;
+ //2. Calculate the sum of acceptable traffic in belowAvgUsageBrokers
+ sumOfAcceptableTrafficFromBelowAvgUsageBrokers.addAndGet(trafficToAccept);
+ belowAvgUsageBrokers.put(broker, brokerData);
+ }
+ if (currentUsage > avgUsage) {
+ //3. Calculate the total traffic in overAvgUsageBrokers
+ totalTrafficOfOverAvgUsageBrokers.addAndGet(brokerCurrentThroughput);
+ overAvgUsageBrokers.put(broker, brokerData);
+ }
+ });
+
+ if (isBalanced.get()) {
+ return selectedBundlesCache;
+ }
+
+ //4. Calculate the percentage of traffic to be migrated by each broker in overAvgUsageBrokers;
+ double percentOfTrafficToOffload = ADDITIONAL_THRESHOLD_PERCENT_MARGIN
Review comment:
Why do we calculate this as a generic average for all brokers? The original calculation of `percentOfTrafficToOffload` is for a specific broker because it is used to determine which bundles to unload to decrease that broker's load to beneath the average. By using an averaged value from all brokers, we could unload too many bundles from some brokers and too few bundles from other brokers.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java
##########
@@ -74,19 +77,56 @@
return selectedBundlesCache;
}
+ AtomicBoolean isBalanced = new AtomicBoolean(true);
+ Map<String, BrokerData> belowAvgUsageBrokers = new HashMap<>();
+ Map<String, BrokerData> overAvgUsageBrokers = new HashMap<>();
+ AtomicDouble sumOfAcceptableTrafficFromBelowAvgUsageBrokers = new AtomicDouble(0);
+ AtomicDouble totalTrafficOfOverAvgUsageBrokers = new AtomicDouble(0);
+
+ // 1. Divided into two categories：① (currentUsage > avgUsage); ②(currentUsage < avgUsage);
loadData.getBrokerData().forEach((broker, brokerData) -> {
+ final LocalBrokerData localData = brokerData.getLocalData();
+ double brokerCurrentThroughput = localData.getMsgThroughputIn() + localData.getMsgThroughputOut();
+ final double currentUsage = brokerAvgResourceUsage.getOrDefault(broker, 0.0);
+
+ if (currentUsage > avgUsage - threshold && currentUsage < avgUsage + threshold) {
+ isBalanced.set(false);
+ return;
+ }
+ if (currentUsage < avgUsage) {
+ double percentOfTrafficToAccept = avgUsage - currentUsage;
+ double trafficToAccept = percentOfTrafficToAccept * brokerCurrentThroughput;
+ //2. Calculate the sum of acceptable traffic in belowAvgUsageBrokers
+ sumOfAcceptableTrafficFromBelowAvgUsageBrokers.addAndGet(trafficToAccept);
+ belowAvgUsageBrokers.put(broker, brokerData);
+ }
+ if (currentUsage > avgUsage) {
+ //3. Calculate the total traffic in overAvgUsageBrokers
+ totalTrafficOfOverAvgUsageBrokers.addAndGet(brokerCurrentThroughput);
+ overAvgUsageBrokers.put(broker, brokerData);
+ }
+ });
+
+ if (isBalanced.get()) {
+ return selectedBundlesCache;
+ }
+
+ //4. Calculate the percentage of traffic to be migrated by each broker in overAvgUsageBrokers;
+ double percentOfTrafficToOffload = ADDITIONAL_THRESHOLD_PERCENT_MARGIN
+ + sumOfAcceptableTrafficFromBelowAvgUsageBrokers.get() / totalTrafficOfOverAvgUsageBrokers.get();
+
+ //5. Select the bundle to unload
+ overAvgUsageBrokers.forEach((broker, brokerData) -> {
final LocalBrokerData localData = brokerData.getLocalData();
final double currentUsage = brokerAvgResourceUsage.getOrDefault(broker, 0.0);
- if (currentUsage < avgUsage + threshold) {
+ if (currentUsage > avgUsage - threshold && currentUsage < avgUsage + threshold) {
Review comment:
Based on the new changes (the brokers being iterated over are all over average), there is no longer a need to update the logic for this conditional.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12471: Optimize ThresholdShedder strategy
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-950132209
> @lordcheng10:Thanks for your contribution. For this PR, do we need to update docs? (The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
No need to add documentation
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12471: Optimize ThresholdShedder strategy
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-950177051
/pulsarbot run-failure-checks
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#issuecomment-974601839
@lordcheng10 can you please check if this PR addresses your concern https://github.com/apache/pulsar/pull/12902
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #12471: Optimize ThresholdShedder strategy: the low-load Broker cannot be fully utilized
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #12471:
URL: https://github.com/apache/pulsar/pull/12471#discussion_r752585894
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java
##########
@@ -74,19 +77,56 @@
return selectedBundlesCache;
}
+ AtomicBoolean isBalanced = new AtomicBoolean(true);
+ Map<String, BrokerData> belowAvgUsageBrokers = new HashMap<>();
+ Map<String, BrokerData> overAvgUsageBrokers = new HashMap<>();
+ AtomicDouble sumOfAcceptableTrafficFromBelowAvgUsageBrokers = new AtomicDouble(0);
+ AtomicDouble totalTrafficOfOverAvgUsageBrokers = new AtomicDouble(0);
+
+ // 1. Divided into two categories：① (currentUsage > avgUsage); ②(currentUsage < avgUsage);
loadData.getBrokerData().forEach((broker, brokerData) -> {
+ final LocalBrokerData localData = brokerData.getLocalData();
+ double brokerCurrentThroughput = localData.getMsgThroughputIn() + localData.getMsgThroughputOut();
+ final double currentUsage = brokerAvgResourceUsage.getOrDefault(broker, 0.0);
+
+ if (currentUsage > avgUsage - threshold && currentUsage < avgUsage + threshold) {
+ isBalanced.set(false);
+ return;
+ }
+ if (currentUsage < avgUsage) {
+ double percentOfTrafficToAccept = avgUsage - currentUsage;
+ double trafficToAccept = percentOfTrafficToAccept * brokerCurrentThroughput;
+ //2. Calculate the sum of acceptable traffic in belowAvgUsageBrokers
+ sumOfAcceptableTrafficFromBelowAvgUsageBrokers.addAndGet(trafficToAccept);
+ belowAvgUsageBrokers.put(broker, brokerData);
+ }
+ if (currentUsage > avgUsage) {
+ //3. Calculate the total traffic in overAvgUsageBrokers
+ totalTrafficOfOverAvgUsageBrokers.addAndGet(brokerCurrentThroughput);
+ overAvgUsageBrokers.put(broker, brokerData);
+ }
+ });
+
+ if (isBalanced.get()) {
+ return selectedBundlesCache;
+ }
+
+ //4. Calculate the percentage of traffic to be migrated by each broker in overAvgUsageBrokers;
+ double percentOfTrafficToOffload = ADDITIONAL_THRESHOLD_PERCENT_MARGIN
Review comment:
> the load utilization rate of all brokers is lower than the average utilization rate.
Can you clarify this point? The average usage cannot be higher than all of the individual usages.
> At this time, according to the original calculation method, there is no way to calculate percentoftraffictooffload.
The current algorithm isn't focused on calculating this value. It only unloads brokers that have utilization above a certain threshold. Moving bundles is not free, so some operators might be fine with a slightly underloaded broker as long as no one broker is too high above the average utilization.
> The sum of the traffic that can be received by all nodes lower than avgusage is the total traffic that we should unload from nodes higher than avgusage. Therefore, we only need to calculate the sum of the total traffic of all brokers higher than avgusage over brokertrafficsum, Then, sumbelowoavgtraffic / overbrokertrafficsum can get the information from each broker that exceeds avgusage
I don't think this is correct. Since brokers in the `overAvgUsageBrokers` collection can have different usages, we need to calculate an amount that is right for _each_ broker itself. Your algorithm might work for your example because all brokers above the average have the same usage.
I think the most productive way to show that your algorithm works as you're saying is to add tests that show how it will react to specific scenarios.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org