Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cli tools] Disable Pulsar client memory limit by default #15748

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented May 24, 2022

Motivation

Modifications

Set .memoryLimit(0, SizeUnit.BYTES) for PulsarClient builders in Cli tools.

Additional context

#15723 restores the default maxPendingMessages & maxPendingMessagesAcrossPartitions limits for producers to the defaults used before PIP-120 changes when the memory limit is disabled. That might be necessary in some cases for restoring the pre-PIP-120 behavior.

- There's a regression with the tools since the memory limit cannot be adjusted
  - It's better to default to the previous setting of disabling memory limits
    so that the performance profile doesn't change because of the memory limit.
@lhotari lhotari force-pushed the lh-disable-memory-limit-in-performance-clients branch from 3e13d58 to 8742024 Compare June 2, 2022 06:57
@lhotari lhotari merged commit a6a7516 into apache:master Jun 2, 2022
@merlimat
Copy link
Contributor

merlimat commented Jun 2, 2022

There's a regression with the tools since the memory limit cannot be adjusted

@lhotari Can you elaborate on what kind of regression it was seen there? I have in general experienced the opposite, going from msg-count based to memory based.

I think this change goes in the wrong direction as it completely removes the memory limit and it will easily cause OOM in the client, since there's no limitation on the producer queue size.

I believe a better change here is to make the memory limit configurable for the perf producer.

@lhotari
Copy link
Member Author

lhotari commented Jun 2, 2022

Can you elaborate on what kind of regression it was seen there? I have in general experienced the opposite, going from msg-count based to memory based.

We have run into regressions with pulsar-perf that seem to be caused by the change in the memory limit. Some test results are very sensitive of the producer's maxPendingMessages and maxPendingMessagesAcrossPartitions settings (as well as batchingMaxBytes, batchingMaxPublishDelayMicros and batchingMaxMessages settings).

For a performance testing tool such as pulsar-perf, I think it's expected that all parameters impacting performance should be tunable. That's why I think it's better to disable the memory limit until there's a configurable parameter.

I believe a better change here is to make the memory limit configurable for the perf producer.

I agree. Until that is done, it's better to set the limit to 0 by default. I added #15912 for tracking this.

lhotari added a commit that referenced this pull request Jun 6, 2022
- There's a regression with the tools since the memory limit cannot be adjusted
  - It's better to default to the previous setting of disabling memory limits
    so that the performance profile doesn't change because of the memory limit.

(cherry picked from commit a6a7516)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 17, 2022
- There's a regression with the tools since the memory limit cannot be adjusted
  - It's better to default to the previous setting of disabling memory limits
    so that the performance profile doesn't change because of the memory limit.

(cherry picked from commit a6a7516)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants