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 2022/09/12 22:09:50 UTC
[GitHub] [pulsar] heesung-sn opened a new pull request, #17598: [improve][loadbalance] added loadBalancerReportUpdateMemoryUsageChecked and loadBalancerReportUpdateMinIntervalMilliSeconds configs
heesung-sn opened a new pull request, #17598:
URL: https://github.com/apache/pulsar/pull/17598
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://docs.google.com/document/d/1d8Pw6ZbWk-_pCKdOmdvx9rnhPiyuxwq60_TrD68d7BA/edit#heading=h.trs9rsex3xom)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
-->
<!-- Either this PR fixes an issue, -->
Fixes #<xyz>
<!-- or this PR is one task of an issue -->
Master Issue: #<xyz>
### Motivation
Currently, the load report frequency is hard-coded at 5secs, and the memory usage is re-computed in the ZK report thread. In the high traffic environment, when GC is frequent, the memory usage could fluctuate in 5 secs, and this could result in writing load report to ZK too frequently. This PR provides configs to tune the minimal frequency of load balance report and to ignore memory usage in the load report update conditions.
<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
### Modifications
<!-- Describe the modifications you've done. -->
- add loadBalancerReportUpdateMemoryUsageChecked (default true)
- add loadBalancerReportUpdateMinIntervalMilliSeconds (default 5000)
The default values are the same as now, so there should be no behavior change after this PR.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Does this pull request potentially affect one of the following parts:
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [x] The default values of configurations
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [x] `doc-required`
(Your PR needs to update docs and you will update later). It needs to update the broker config doc. https://pulsar.apache.org/docs/reference-configuration/#broker
- [] `doc-not-needed`
(Please explain why) loadBalancerReportUpdateMinIntervalMilliSeconds
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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] Anonymitaet commented on pull request #17598: [improve][loadbalance] added loadBalancerReportUpdateMinIntervalMillis and ignores memory usage in getMaxResourceUsage()
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on PR #17598:
URL: https://github.com/apache/pulsar/pull/17598#issuecomment-1255772341
> @Anonymitaet the configuration doc update can be automated in the next version, which means we don't need to update https://pulsar.apache.org/docs/reference-configuration/#broker manually, correct?
yes
--
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] heesung-sn commented on pull request #17598: [improve][loadbalance] added loadBalancerReportUpdateMemoryUsageChecked and ignores memory usage in getMaxResourceUsage()
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on PR #17598:
URL: https://github.com/apache/pulsar/pull/17598#issuecomment-1244919192
/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] momo-jun commented on pull request #17598: [improve][loadbalance] added loadBalancerReportUpdateMinIntervalMillis and ignores memory usage in getMaxResourceUsage()
Posted by GitBox <gi...@apache.org>.
momo-jun commented on PR #17598:
URL: https://github.com/apache/pulsar/pull/17598#issuecomment-1255770339
@Anonymitaet the configuration doc update can be automated in the next version, which means we don't need to update https://pulsar.apache.org/docs/reference-configuration/#broker manually, correct?
--
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] heesung-sn commented on a diff in pull request #17598: [improve][loadbalance] added loadBalancerReportUpdateMemoryUsageChecked and ignores memory usage in getMaxResourceUsage()
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on code in PR #17598:
URL: https://github.com/apache/pulsar/pull/17598#discussion_r969050855
##########
pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java:
##########
@@ -235,7 +235,16 @@ private void updateBundleData(final Map<String, NamespaceBundleStats> bundleStat
}
public double getMaxResourceUsage() {
- return max(cpu.percentUsage(), memory.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
+ return getMaxResourceUsage(true);
+ }
+
+ public double getMaxResourceUsage(boolean checkMemoryUsage) {
+ if (checkMemoryUsage) {
Review Comment:
Updated.
I initially thought this checkMemoryUsage config is safer since it does keep the old behavior(checking memory).
I see that memory usage is always noisy, so I agree that we could remove it from `getMaxResourceUsage`.
Note that we keep `getMaxResourceUsageWithWeightWithinLimit()` as-is -- we are not removing the memory usage signal in this func, as its memory usage can be already disabled by `loadBalancerMemoryResourceWeight=0.0`.
--
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] merlimat commented on a diff in pull request #17598: [improve][loadbalance] added loadBalancerReportUpdateMemoryUsageChecked and loadBalancerReportUpdateMinIntervalMilliSeconds configs
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #17598:
URL: https://github.com/apache/pulsar/pull/17598#discussion_r969049493
##########
pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java:
##########
@@ -235,7 +235,16 @@ private void updateBundleData(final Map<String, NamespaceBundleStats> bundleStat
}
public double getMaxResourceUsage() {
- return max(cpu.percentUsage(), memory.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
+ return getMaxResourceUsage(true);
+ }
+
+ public double getMaxResourceUsage(boolean checkMemoryUsage) {
+ if (checkMemoryUsage) {
Review Comment:
I would suggest to remove this config option and instead only check the direct memory but not the heap memory usage here.
--
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] codelipenghui merged pull request #17598: [improve][loadbalance] added loadBalancerReportUpdateMinIntervalMillis and ignores memory usage in getMaxResourceUsage()
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #17598:
URL: https://github.com/apache/pulsar/pull/17598
--
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] codelipenghui commented on a diff in pull request #17598: [improve][loadbalance] added loadBalancerReportUpdateMemoryUsageChecked and ignores memory usage in getMaxResourceUsage()
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #17598:
URL: https://github.com/apache/pulsar/pull/17598#discussion_r969625275
##########
pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java:
##########
@@ -235,7 +235,8 @@ private void updateBundleData(final Map<String, NamespaceBundleStats> bundleStat
}
public double getMaxResourceUsage() {
- return max(cpu.percentUsage(), memory.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
+ // does not consider memory because it is noisy by gc.
+ return max(cpu.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
Review Comment:
Could you please help add a unit test?
Looks like we changed the behavior, but all the tests got passed.
##########
conf/broker.conf:
##########
@@ -1163,6 +1163,9 @@ loadBalancerEnabled=true
# Percentage of change to trigger load report update
loadBalancerReportUpdateThresholdPercentage=10
+# minimum interval to update load report
+loadBalancerReportUpdateMinIntervalMilliSeconds=5000
Review Comment:
```suggestion
loadBalancerReportUpdateMinIntervalMillis=5000
```
Just keep consistent with other configuration names in the broker.conf
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2087,6 +2087,14 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "maximum interval to update load report"
)
+
+ private int loadBalancerReportUpdateMinIntervalMilliSeconds = 5000;
+ @FieldContext(
+ category = CATEGORY_LOAD_BALANCER,
+ dynamic = true,
+ doc = "Min delay of load report to collect, in milli-seconds"
+ )
Review Comment:
Should move to line 2090?
--
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] heesung-sn commented on a diff in pull request #17598: [improve][loadbalance] added loadBalancerReportUpdateMemoryUsageChecked and ignores memory usage in getMaxResourceUsage()
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on code in PR #17598:
URL: https://github.com/apache/pulsar/pull/17598#discussion_r970151813
##########
pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java:
##########
@@ -235,7 +235,8 @@ private void updateBundleData(final Map<String, NamespaceBundleStats> bundleStat
}
public double getMaxResourceUsage() {
- return max(cpu.percentUsage(), memory.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
+ // does not consider memory because it is noisy by gc.
+ return max(cpu.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
Review Comment:
updated.
##########
conf/broker.conf:
##########
@@ -1163,6 +1163,9 @@ loadBalancerEnabled=true
# Percentage of change to trigger load report update
loadBalancerReportUpdateThresholdPercentage=10
+# minimum interval to update load report
+loadBalancerReportUpdateMinIntervalMilliSeconds=5000
Review Comment:
updated.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2087,6 +2087,14 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "maximum interval to update load report"
)
+
+ private int loadBalancerReportUpdateMinIntervalMilliSeconds = 5000;
+ @FieldContext(
+ category = CATEGORY_LOAD_BALANCER,
+ dynamic = true,
+ doc = "Min delay of load report to collect, in milli-seconds"
+ )
Review Comment:
updated.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2087,6 +2087,14 @@ public class ServiceConfiguration implements PulsarConfiguration {
category = CATEGORY_LOAD_BALANCER,
doc = "maximum interval to update load report"
)
+
+ private int loadBalancerReportUpdateMinIntervalMilliSeconds = 5000;
+ @FieldContext(
+ category = CATEGORY_LOAD_BALANCER,
+ dynamic = true,
+ doc = "Min delay of load report to collect, in milli-seconds"
+ )
Review Comment:
updated.
##########
pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java:
##########
@@ -235,7 +235,8 @@ private void updateBundleData(final Map<String, NamespaceBundleStats> bundleStat
}
public double getMaxResourceUsage() {
- return max(cpu.percentUsage(), memory.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
+ // does not consider memory because it is noisy by gc.
+ return max(cpu.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
Review Comment:
updated.
--
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] heesung-sn commented on a diff in pull request #17598: [improve][loadbalance] added loadBalancerReportUpdateMemoryUsageChecked and loadBalancerReportUpdateMinIntervalMilliSeconds configs
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on code in PR #17598:
URL: https://github.com/apache/pulsar/pull/17598#discussion_r969050855
##########
pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java:
##########
@@ -235,7 +235,16 @@ private void updateBundleData(final Map<String, NamespaceBundleStats> bundleStat
}
public double getMaxResourceUsage() {
- return max(cpu.percentUsage(), memory.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
+ return getMaxResourceUsage(true);
+ }
+
+ public double getMaxResourceUsage(boolean checkMemoryUsage) {
+ if (checkMemoryUsage) {
Review Comment:
Updated.
--
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] heesung-sn commented on a diff in pull request #17598: [improve][loadbalance] added loadBalancerReportUpdateMemoryUsageChecked and ignores memory usage in getMaxResourceUsage()
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on code in PR #17598:
URL: https://github.com/apache/pulsar/pull/17598#discussion_r969050855
##########
pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java:
##########
@@ -235,7 +235,16 @@ private void updateBundleData(final Map<String, NamespaceBundleStats> bundleStat
}
public double getMaxResourceUsage() {
- return max(cpu.percentUsage(), memory.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
+ return getMaxResourceUsage(true);
+ }
+
+ public double getMaxResourceUsage(boolean checkMemoryUsage) {
+ if (checkMemoryUsage) {
Review Comment:
Updated.
I initially thought this checkMemoryUsage config is safer since it does keep the old behavior(checking memory).
I see that memory usage is always noisy, so I agree that we could remove it from `getMaxResourceUsage`.
Note that we keep `getMaxResourceUsageWithWeightWithinLimit()` as-is -- we are not removing the memory usage signal in this func, as its memory usage is already configurable by `loadBalancerMemoryResourceWeight=0.0`.
--
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] heesung-sn commented on pull request #17598: [improve][loadbalance] added loadBalancerReportUpdateMinIntervalMillis and ignores memory usage in getMaxResourceUsage()
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on PR #17598:
URL: https://github.com/apache/pulsar/pull/17598#issuecomment-1246086175
/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] heesung-sn commented on pull request #17598: [improve][loadbalance] added loadBalancerReportUpdateMinIntervalMillis and ignores memory usage in getMaxResourceUsage()
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on PR #17598:
URL: https://github.com/apache/pulsar/pull/17598#issuecomment-1246125108
/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