[fix][cli] Restore the 64M default client memory limit in pulsar-perf - #26341
[fix][cli] Restore the 64M default client memory limit in pulsar-perf#26341AlvaroStream wants to merge 1 commit into
Conversation
Fixes apache#26340 Before apache#20663, pulsar-perf never called ClientBuilder#memoryLimit, so it inherited the client default of 64M from ClientConfigurationData. That PR added a --memory-limit option and applied it unconditionally, but the backing field has no initializer, so an unset option passes 0. Since MemoryLimitController#isMemoryLimited is memoryLimit > 0, 0 disables the limit entirely, and pulsar-perf silently went from bounded to unbounded client memory. With the limit disabled, a producer that outruns the brokers accumulates outbound buffers without backpressure until direct memory is exhausted, failing with OutOfDirectMemoryError instead of throttling. Default memoryLimit to the client's own default and extract that default into ClientConfigurationData.DEFAULT_MEMORY_LIMIT_BYTES so the two cannot drift. Passing --memory-limit 0 still disables the limit explicitly.
| + "(eg: 32M, 64M). Use 0 to disable the limit. Default: 64M", | ||
| converter = ByteUnitToLongConverter.class) | ||
| public long memoryLimit = DEFAULT_MEMORY_LIMIT_BYTES; |
There was a problem hiding this comment.
Before changing this, we'd need to find the actual root cause since this change will cause different pulsar-perf results in certain cases. This has happened in the past with changes in #13344. #15723 and #15748 were made at that time to address the performance regression, #15748 has some context.
Although setting the memory limit will bound the memory usage, it changes the results. For users using specific parameters with an older version of the tool will get different results where the backpressure is expected to be applied by maxPendingMessages and maxPendingMessagesAcrossPartitions.
The intention of this change has been to apply backpressure:
The possible root cause could be that this regresses in some way between 3.0.5 and 4.0.9.
There was a problem hiding this comment.
I performed analysis with Claude and based on that, it seems that the NO_MEMORY_LIMIT_DEFAULT_MAX_PENDING_MESSAGES / NO_MEMORY_LIMIT_DEFAULT_MAX_PENDING_MESSAGES_ACROSS_PARTITIONS doesn't get applied for non-partitioned topics.
If we change the default memory limit for pulsar-perf, I believe that the limit should be proportional to the max direct memory provided to pulsar-perf. The reason for this is that it would prevent silent performance regressions caused by the parameter change while capping the memory limit and preventing OOM.
Since pulsar-perf doesn't use direct memory for other purposes than Netty, it could use 50% of available direct memory.
Code example of setting a parameter based on available direct memory:
There was a problem hiding this comment.
There was a problem hiding this comment.
Should we make it the default so test can be compared and improve it later?
There was a problem hiding this comment.
Should we make it the default so test can be compared and improve it later?
I'd suggest taking the path where instead of setting the memory limit to 64M, it would be 50% of available direct memory.
This would make pulsar-perf test results more consistent across versions since there hasn't been a limit in the past. Setting a limit changes the behavior significantly for many workloads since there will be less inflight messages. This would mainly impact tests where there are a lot of partitions and the message sizes are relatively large (for example 100 partitions, 32kB message size, very high message rate).
Making the default proportional to the available direct memory is useful since the direct memory would actually get used when there's available memory to use.
The problem with the previous NO_MEMORY_LIMIT_DEFAULT_MAX_PENDING_MESSAGES/NO_MEMORY_LIMIT_DEFAULT_MAX_PENDING_MESSAGES_ACROSS_PARTITIONS solution was that it didn't apply to non-partitioned topics. That's fixed by #26342. Since those limits have been in place for partitioned topics, setting the memory limit to 64M change the behavior and produce different results.
Hopefully this clarifies the reason why setting to 64M isn't something that I support and I'm instead recommending to make it dynamic, based on available direct memory.
It's a very simple change to this PR to make it dynamic. (long) (0.5d * DirectMemoryUtils.jvmMaxDirectMemory()) will return 50% of total direct memory in bytes.
|
For Pulsar 4.x, addressing #26340 needs #26371 and #26342 in addition to this one. On 4.x, One correction to what I said earlier: the That is why the 4.x fix takes all three: this PR restores the byte-based backpressure; #26371 makes #26371 targets branch-4.2; branch-4.0 and branch-4.1 have the identical |
Fixes #26340
Motivation
pulsar-perfcan exhaust direct memory and die withOutOfDirectMemoryErrorwhenever theproducer outruns the brokers:
The same run against a 3.0.x client completes normally, which makes this look like a client
regression. It is, but not in the allocator. It is a lost default.
Before #20663,
PerfClientUtils.createClientBuilderFromArgumentsnever calledClientBuilder#memoryLimit, sopulsar-perfinherited the client default of 64M fromClientConfigurationData#memoryLimitBytes.That PR added the
--memory-limitoption and applied it unconditionally:but the backing field has no initializer:
so when the option is not supplied,
pulsar-perfpasses0. And0is not "no override", it is"no limit":
So
pulsar-perfwent from bounded to unbounded client memory without anyone choosing that.With the limit disabled there is no backpressure on the producer. When the brokers cannot keep up
with the offered rate, outbound buffers accumulate until direct memory is exhausted and the client
dies, rather than throttling and reporting the achievable rate, which is what a benchmarking tool
should do. Passing
--memory-limit 64Mrestores the old behaviour and the same run completes.This also affects
PerformanceConsumer,PerformanceReader,PerformanceTransactionandLoadSimulationClient, which sharePerformanceBaseArguments, and on master it reaches both theexisting builder and the v5
PulsarClientBuildercall site.Modifications
ClientConfigurationData.DEFAULT_MEMORY_LIMIT_BYTESand use itfor
memoryLimitBytes. This is a pure refactor of a value that is already 64M; it just gives thedefault a name so the CLI can reference it. It follows the existing convention in the sibling
ProducerConfigurationData, which already exposesDEFAULT_BATCHING_MAX_MESSAGESand friendsthat
PerformanceProducerimports.PerformanceBaseArguments#memoryLimitto that constant, so an unset--memory-limitreproduces the pre-[feat][cli] Add command line option for configuring the memory limit #20663 behaviour instead of silently disabling the limit. Referencing the
constant rather than repeating
64 * 1024 * 1024means the CLI default cannot drift from theclient default.
0disables the limit.--memory-limit 0still disables the limit explicitly, so the capability added by #20663 isretained. Only the unset behaviour changes.
Verifying this change
This change is already covered by existing tests, such as
PerformanceBaseArgumentsTest#testMemoryLimitCliArgument, which continues to verify that explicitvalues (
-ml 1,-ml 1K,--memory-limit 1G) are parsed correctly.PerformanceBaseArgumentsTest#testMemoryLimitCliArgumentDefaultasserted the previous0defaultand is updated to assert the restored 64M default.
Added
PerformanceBaseArgumentsTest#testMemoryLimitCanBeDisabledto pin the escape hatch, so afuture change cannot quietly remove the ability to run unbounded via
-ml 0.The end-to-end behaviour was verified against a 3-node cluster:
pulsar-perf produce -r 100000 -time 300 -s 1024fails withOutOfDirectMemoryErroron 4.0.9 and 4.0.13, and completes once the64M limit is in effect.
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
The default of the
pulsar-perf--memory-limitoption changes from0(unlimited) to64M.This restores the behaviour that
pulsar-perfhad before #20663 rather than introducing a newone, and it aligns the tool with the documented client default. Existing runs that relied on
unbounded client memory need
--memory-limit 0to keep it.ClientConfigurationData#memoryLimitBytesitself is unchanged at 64M; only the literal moves to anamed constant.