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

[315157973]

Motivation

Even for a Broker with a very low load, its memory will grow slowly until GC is triggered. If memory is used as a load calculation item, the Bundle will be unloaded by mistake

Modifications

Remove memory as load calculation item

Verifying this change

Existing unit tests need to pass all
315157973#5

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

[Technoboy-]

LGTM

[codecov-commenter]

Codecov Report

Merging #19559 (c3b2c20) into master (fe547c7) will increase coverage by 33.69%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             master   #19559       +/-   ##
=============================================
+ Coverage     28.70%   62.40%   +33.69%     
- Complexity     6986    25878    +18892     
=============================================
  Files          1670     1844      +174     
  Lines        125950   135279     +9329     
  Branches      13728    14875     +1147     
=============================================
+ Hits          36159    84418    +48259     
+ Misses        84392    43103    -41289     
- Partials       5399     7758     +2359     

Flag	Coverage Δ
inttests	24.58% <100.00%> (?)
systests	25.32% <100.00%> (-0.01%)	⬇️
unittests	59.47% <100.00%> (+40.00%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...loadbalance/impl/LeastResourceUsageWithWeight.java	0.00% <ø> (ø)
...lsar/broker/loadbalance/impl/ThresholdShedder.java	31.14% <100.00%> (+4.09%)	⬆️
...ar/policies/data/loadbalancer/LocalBrokerData.java	92.85% <100.00%> (ø)
...ache/pulsar/zookeeper/LocalBookkeeperEnsemble.java	46.50% <0.00%> (-16.09%)	⬇️
...r/service/AbstractDispatcherMultipleConsumers.java	70.96% <0.00%> (-9.68%)	⬇️
...r/service/schema/DefaultSchemaRegistryService.java	0.00% <0.00%> (-6.25%)	⬇️
...g/apache/pulsar/broker/service/StreamingStats.java	89.18% <0.00%> (-5.41%)	⬇️
...in/java/org/apache/pulsar/PulsarBrokerStarter.java	28.14% <0.00%> (-3.00%)	⬇️
...che/pulsar/broker/BookKeeperClientFactoryImpl.java	50.46% <0.00%> (-2.81%)	⬇️
...tion/pendingack/impl/PendingAckHandleDisabled.java	29.41% <0.00%> (-1.84%)	⬇️
... and 1255 more

[michaeljmarshall]

Please discuss this change on the mailing list. This is a fundamental change.

[315157973]

Please discuss this change on the mailing list. This is a fundamental change.

I don't think it is a fundamental change.
This is an obvious bug , memory is not used in the getMaxResourceUsage method in the same class.
Two methods to get MaxResourceUsage, but the behavior is different

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

Lines 239 to 243 in 660525e

	public double getMaxResourceUsage() {
	// does not consider memory because it is noisy by gc.
	return max(cpu.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
	bandwidthOut.percentUsage()) / 100;
	}

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

Lines 253 to 259 in 660525e

	public double getMaxResourceUsageWithWeight(final double cpuWeight, final double memoryWeight,
	final double directMemoryWeight, final double bandwidthInWeight,
	final double bandwidthOutWeight) {
	return max(cpu.percentUsage() * cpuWeight, memory.percentUsage() * memoryWeight,
	directMemory.percentUsage() * directMemoryWeight, bandwidthIn.percentUsage() * bandwidthInWeight,
	bandwidthOut.percentUsage() * bandwidthOutWeight) / 100;
	}

[Jason918]

Should we deprecated the config loadBalancerMemoryResourceWeight first? Or change its default value = 0.

[michaeljmarshall]

Please discuss this change on the mailing list. This is a fundamental change.

I don't think it is a fundamental change. This is an obvious bug , memory is not used in the getMaxResourceUsage method in the same class. Two methods to get MaxResourceUsage, but the behavior is different

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

Lines 239 to 243 in 660525e

	public double getMaxResourceUsage() {
	// does not consider memory because it is noisy by gc.
	return max(cpu.percentUsage(), directMemory.percentUsage(), bandwidthIn.percentUsage(),
	bandwidthOut.percentUsage()) / 100;
	}

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

Lines 253 to 259 in 660525e

	public double getMaxResourceUsageWithWeight(final double cpuWeight, final double memoryWeight,
	final double directMemoryWeight, final double bandwidthInWeight,
	final double bandwidthOutWeight) {
	return max(cpu.percentUsage() * cpuWeight, memory.percentUsage() * memoryWeight,
	directMemory.percentUsage() * directMemoryWeight, bandwidthIn.percentUsage() * bandwidthInWeight,
	bandwidthOut.percentUsage() * bandwidthOutWeight) / 100;
	}

Thanks for the references. This justification would have been very helpful to provide in the PR description.

I missed #17598, which made the inconsistent change that you reference. Before #17598, we used memory for both methods. That change could certainly have had a note on the mailing list since it is a fundamental change to how the load balancer works.

This change will affect the LeastResourceUsageWithWeight and the ThresholdShedder.

Interestingly, #17598 addresses the content of this PR and also addresses @Jason918's comment:

Note that this PR does not update getMaxResourceUsageWithWeight*(), as it already controls the memory usage by loadBalancerMemoryResourceWeight=0.0

However, from what I see, loadBalancerMemoryResourceWeight defaults to 1, not 0.

[michaeljmarshall]

Should we deprecated the config loadBalancerMemoryResourceWeight first? Or change its default value = 0.

I think this is the right direction to take with this PR in order to make sure that we do not break user implementations.

[eolivelli]

I agree that we should not remove the methods but deprecated them.

[michaeljmarshall]

LGTM. I think we might want to update some additional documentation, though.