[improve][loadbalance] added loadBalancerReportUpdateMinIntervalMillis and ignores memory usage in getMaxResourceUsage() #17598

heesung-sn · 2022-09-12T22:09:48Z

Fixes #

Master Issue: #

Motivation

Currently, the load report frequency is hard-coded at 5secs, and the JVM memory usage is re-computed in the ZK report thread every 5 sec. In the high traffic environment, when GC is frequent, the memory usage could fluctuate in 5 secs, and this could result in writing load reports to ZK too frequently(every 5 sec).

Modifications

Adds a config, loadBalancerReportUpdateMinIntervalMilliSeconds (default 5000) to configure the minimum frequency of the load update to ZK instead of hard-coded 5secs.
Removes the memory usage check in getMaxResourceUsage() to ignore the noisy memory usage signals.

Currently, there are three places where Pulsar uses getMaxResourceUsage()

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/OverloadShedder.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LeastLongTermMessageRate.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java

Search Result: https://github.com/apache/pulsar/search?q=getMaxResourceUsage
Note that this PR does not update getMaxResourceUsageWithWeight*(), as it already controls the memory usage by loadBalancerMemoryResourceWeight=0.0

Verifying this change

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

Documentation

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)

merlimat · 2022-09-13T01:04:58Z

pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java

+    }
+
+    public double getMaxResourceUsage(boolean checkMemoryUsage) {
+        if (checkMemoryUsage) {


I would suggest to remove this config option and instead only check the direct memory but not the heap memory usage here.

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.

heesung-sn · 2022-09-13T05:32:00Z

/pulsarbot run-failure-checks

codelipenghui

And could you please also add a test for the new configuration?

codelipenghui · 2022-09-13T13:20:57Z

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


Suggested change

loadBalancerReportUpdateMinIntervalMilliSeconds=5000

loadBalancerReportUpdateMinIntervalMillis=5000

Just keep consistent with other configuration names in the broker.conf

codelipenghui · 2022-09-13T13:23:05Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

+    @FieldContext(
+            category = CATEGORY_LOAD_BALANCER,
+            dynamic = true,
+            doc = "Min delay of load report to collect, in milli-seconds"
+    )


Should move to line 2090?

codelipenghui · 2022-09-13T13:25:25Z

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(),


Could you please help add a unit test?
Looks like we changed the behavior, but all the tests got passed.

… config and ignores memory usage in getMaxResourceUsage$()

heesung-sn · 2022-09-14T00:30:06Z

/pulsarbot run-failure-checks

heesung-sn · 2022-09-14T01:41:57Z

/pulsarbot run-failure-checks

…s and ignores memory usage in getMaxResourceUsage() (#17598) (cherry picked from commit de7c586)

momo-jun · 2022-09-23T03:46:18Z

@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?

Anonymitaet · 2022-09-23T03:52:26Z

@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

…s and ignores memory usage in getMaxResourceUsage() (apache#17598) (cherry picked from commit de7c586) (cherry picked from commit a6cb786)

github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Sep 12, 2022

merlimat reviewed Sep 13, 2022

View reviewed changes

heesung-sn force-pushed the update-lb-update-frequency-config branch from e0d963d to 94ad0f9 Compare September 13, 2022 01:07

heesung-sn changed the title ~~[improve][loadbalance] added loadBalancerReportUpdateMemoryUsageChecked and loadBalancerReportUpdateMinIntervalMilliSeconds configs~~ [improve][loadbalance] added loadBalancerReportUpdateMemoryUsageChecked and ignores memory usage in getMaxResourceUsage() Sep 13, 2022

merlimat approved these changes Sep 13, 2022

View reviewed changes

merlimat added this to the 2.12.0 milestone Sep 13, 2022

merlimat added release/2.9.4 release/2.11.1 release/2.10.3 type/bug The PR fixed a bug or issue reported a bug labels Sep 13, 2022

merlimat assigned heesung-sn Sep 13, 2022

Demogorgon314 approved these changes Sep 13, 2022

View reviewed changes

codelipenghui requested changes Sep 13, 2022

View reviewed changes

[improve][loadbalance] Adds loadBalancerReportUpdateMinIntervalMillis…

5844002

… config and ignores memory usage in getMaxResourceUsage$()

heesung-sn force-pushed the update-lb-update-frequency-config branch from 94ad0f9 to 5844002 Compare September 13, 2022 23:02

codelipenghui approved these changes Sep 13, 2022

View reviewed changes

heesung-sn changed the title ~~[improve][loadbalance] added loadBalancerReportUpdateMemoryUsageChecked and ignores memory usage in getMaxResourceUsage()~~ [improve][loadbalance] added loadBalancerReportUpdateMinIntervalMillis and ignores memory usage in getMaxResourceUsage() Sep 14, 2022

codelipenghui merged commit de7c586 into apache:master Sep 14, 2022

codelipenghui added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker labels Sep 14, 2022

heesung-sn mentioned this pull request Sep 16, 2022

[improve][loadbalance] Adds loadBalancerReportUpdateMinIntervalMilliS… heesung-sn/pulsar#7

Closed

13 tasks

codelipenghui pushed a commit that referenced this pull request Sep 21, 2022

[improve][loadbalance] added loadBalancerReportUpdateMinIntervalMilli…

0058e73

…s and ignores memory usage in getMaxResourceUsage() (#17598) (cherry picked from commit de7c586)

codelipenghui pushed a commit that referenced this pull request Sep 21, 2022

[improve][loadbalance] added loadBalancerReportUpdateMinIntervalMilli…

aa4c677

…s and ignores memory usage in getMaxResourceUsage() (#17598) (cherry picked from commit de7c586)

codelipenghui added cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 labels Sep 21, 2022

codelipenghui removed this from the 2.12.0 milestone Sep 21, 2022

codelipenghui added this to the 2.11.0 milestone Sep 21, 2022

codelipenghui removed the release/2.11.1 label Sep 21, 2022

codelipenghui pushed a commit that referenced this pull request Sep 21, 2022

[improve][loadbalance] added loadBalancerReportUpdateMinIntervalMilli…

a6cb786

…s and ignores memory usage in getMaxResourceUsage() (#17598) (cherry picked from commit de7c586)

momo-jun added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Sep 23, 2022

Jason918 added release/2.10.2 and removed release/2.10.3 labels Sep 26, 2022

michaeljmarshall mentioned this pull request Feb 22, 2023

[fix][broker] Remove useless load balancer items about MemoryResourceWeight #19559

Merged

4 tasks

heesung-sn deleted the update-lb-update-frequency-config branch April 2, 2024 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][loadbalance] added loadBalancerReportUpdateMinIntervalMillis and ignores memory usage in getMaxResourceUsage() #17598

[improve][loadbalance] added loadBalancerReportUpdateMinIntervalMillis and ignores memory usage in getMaxResourceUsage() #17598

heesung-sn commented Sep 12, 2022 •

edited by momo-jun

Loading

merlimat Sep 13, 2022

heesung-sn Sep 13, 2022 •

edited

Loading

heesung-sn commented Sep 13, 2022

codelipenghui left a comment

codelipenghui Sep 13, 2022

heesung-sn Sep 13, 2022

codelipenghui Sep 13, 2022

heesung-sn Sep 13, 2022

codelipenghui Sep 13, 2022

heesung-sn Sep 13, 2022

heesung-sn commented Sep 14, 2022

heesung-sn commented Sep 14, 2022

momo-jun commented Sep 23, 2022

Anonymitaet commented Sep 23, 2022

	loadBalancerReportUpdateMinIntervalMilliSeconds=5000
	loadBalancerReportUpdateMinIntervalMillis=5000

[improve][loadbalance] added loadBalancerReportUpdateMinIntervalMillis and ignores memory usage in getMaxResourceUsage() #17598

[improve][loadbalance] added loadBalancerReportUpdateMinIntervalMillis and ignores memory usage in getMaxResourceUsage() #17598

Conversation

heesung-sn commented Sep 12, 2022 • edited by momo-jun Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

merlimat Sep 13, 2022

Choose a reason for hiding this comment

heesung-sn Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

heesung-sn commented Sep 13, 2022

codelipenghui left a comment

Choose a reason for hiding this comment

codelipenghui Sep 13, 2022

Choose a reason for hiding this comment

heesung-sn Sep 13, 2022

Choose a reason for hiding this comment

codelipenghui Sep 13, 2022

Choose a reason for hiding this comment

heesung-sn Sep 13, 2022

Choose a reason for hiding this comment

codelipenghui Sep 13, 2022

Choose a reason for hiding this comment

heesung-sn Sep 13, 2022

Choose a reason for hiding this comment

heesung-sn commented Sep 14, 2022

heesung-sn commented Sep 14, 2022

momo-jun commented Sep 23, 2022

Anonymitaet commented Sep 23, 2022

heesung-sn commented Sep 12, 2022 •

edited by momo-jun

Loading

heesung-sn Sep 13, 2022 •

edited

Loading