Skip to content

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

Merged
lhotari merged 1 commit into
apache:masterfrom
lhotari:lh-disable-memory-limit-in-performance-clients
Jun 2, 2022
Merged

[Cli tools] Disable Pulsar client memory limit by default#15748
lhotari merged 1 commit into
apache:masterfrom
lhotari:lh-disable-memory-limit-in-performance-clients

Conversation

@lhotari

@lhotari lhotari commented May 24, 2022

Copy link
Copy Markdown
Member

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

merlimat commented Jun 2, 2022

Copy link
Copy Markdown
Contributor

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

lhotari commented Jun 2, 2022

Copy link
Copy Markdown
Member Author

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