PIP-120 : Enable client memory limit controller by default #13344

HQebupt · 2021-12-15T16:13:11Z

Motivation

Modifications

Enable the MemeoryLimitController. The memoryLimitBytes default is 64M which is easier to control than the maxPendingMessages.

The default 64M has a good performance in produce throughput in our perf test. I am going to post the performance result here.

Verifying this change

Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

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

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

doc

(There is description in the configuration modification)

merlimat

Let's wait until PIP is accepted before merging

merlimat · 2021-12-15T16:39:14Z

pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ProducerBuilder.java

@@ -172,7 +172,7 @@
     * the client application. Until, the producer gets a successful acknowledgment back from the broker,
     * it will keep in memory (direct memory pool) all the messages in the pending queue.
     *
-     * <p>Default is 1000.
+     * <p>Default is 0, disable the pending messages check.


We should also deprecate maxPendingMessagesAcrossPartitions() (and setting it to 0 by default), because memory limit is a better approach to limit the max amount of memory.

Done. The maxPendingMessagesAcrossPartitions and maxPendingMessages are deprecated.

Oh, but let's not deprecate this one (maxPendingMessages), since there will still be legitimate reasons to set a max number of messages.

github-actions · 2021-12-15T17:00:03Z

@HQebupt:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

shoothzj · 2021-12-16T00:29:10Z

the test org.apache.pulsar.client.impl.ProducerBuilderImplTest#testProducerBuilderImplWhenMaxPendingMessagesAcrossPartitionsPropertyIsInvalid need to be modified too

HQebupt · 2021-12-16T03:53:22Z

the test org.apache.pulsar.client.impl.ProducerBuilderImplTest#testProducerBuilderImplWhenMaxPendingMessagesAcrossPartitionsPropertyIsInvalid need to be modified too

@shoothzj Good suggestion. Done. 👍

HQebupt · 2021-12-16T03:53:51Z

@HQebupt:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

Yes

HQebupt · 2021-12-16T05:00:07Z

/pulsarbot run-failure-checks

Anonymitaet · 2021-12-16T08:23:20Z

Hi @HQebupt When submitting a PR, please provide doc related info in the PR description by ticking the box or labeling a PR directly, so that Bot will recognize the info and then label the PR correctly, or else Bot can not recognize the info and then label the PR with the doc-info-missing label. More info see https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0/edit#

I've labeled this PR with doc, please pay attention next time, thanks.

HQebupt · 2021-12-16T14:15:59Z

Hi @HQebupt When submitting a PR, please provide doc related info in the PR description by ticking the box or labeling a PR directly, so that Bot will recognize the info and then label the PR correctly, or else Bot can not recognize the info and then label the PR with the doc-info-missing label. More info see https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0/edit#

I've labeled this PR with doc, please pay attention next time, thanks.

@Anonymitaet Thanks a lot. I realized I did not find the right way.

HQebupt · 2021-12-16T14:16:31Z

/pulsarbot run-failure-checks

HQebupt · 2021-12-18T05:27:00Z

/pulsarbot run-failure-checks

yangl · 2021-12-23T02:06:52Z

@HQebupt @merlimat There's no problem setting the memoryLimit by default, but I don't think it's appropriate to deprecate maxPendingMessages right now.

merlimat · 2021-12-23T02:21:29Z

@yangl You're right, we shouldn't deprecate the maxPendingMessages

HQebupt · 2021-12-23T12:35:36Z

Good, let me revert it. I keep the default value as 0.

HQebupt · 2021-12-24T10:03:20Z

/pulsarbot run-failure-checks

HQebupt · 2022-01-22T15:28:41Z

Thanks @merlimat

…ents ### Motivation After apache#13344, we have changed the default value of maxPendingMessagesAcrossPartitions to `0`, for the performance producer, it will always apply the default value of maxPendingMessagesAcrossPartitions while creating producers, so it will always use `0` for maxPendingMessagesAcrossPartitions. But we have a check here https://github.com/apache/pulsar/blob/0a91196dcc4d31ae647867ed319b8c1af0cb93c6/pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ProducerConfigurationData.java#L138-L142 This will lead the performance producer not able to create if users only set `-o` option ``` 06:32:49.628 [pulsar-perf-producer-exec-1-1] ERROR org.apache.pulsar.testclient.PerformanceProducer - Got error java.lang.IllegalArgumentException: maxPendingMessagesAcrossPartitions needs to be >= maxPendingMessages at org.apache.pulsar.shade.com.google.common.base.Preconditions.checkArgument(Preconditions.java:145) ~[pulsar-client-admin-2.10.0.2.jar:2.10.0.2] at org.apache.pulsar.client.impl.conf.ProducerConfigurationData.setMaxPendingMessagesAcrossPartitions(ProducerConfigurationData.java:138) ~[pulsar-client-admin-2.10.0.2.jar:2.10.0.2] at org.apache.pulsar.client.impl.ProducerBuilderImpl.maxPendingMessagesAcrossPartitions(ProducerBuilderImpl.java:148) ~[pulsar-client-admin-2.10.0.2.jar:2.10.0.2] at org.apache.pulsar.testclient.PerformanceProducer.runProducer(PerformanceProducer.java:600) ~[pulsar-testclient-2.10.0.2.jar:2.10.0.2] at org.apache.pulsar.testclient.PerformanceProducer.lambda$main$1(PerformanceProducer.java:464) ~[pulsar-testclient-2.10.0.2.jar:2.10.0.2] at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [?:1.8.0_332] at java.util.concurrent.FutureTask.run(FutureTask.java:266) [?:1.8.0_332] at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_332] at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_332] at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [netty-common-4.1.74.Final.jar:4.1.74.Final] at java.lang.Thread.run(Thread.java:750) [?:1.8.0_332] ``` ### Modification Change the performance producer to only call `.maxPendingMessagesAcrossPartitions()` if it presents. ### Verification Added new test to only use `-o` to start the performance producer, use a consumer to consume data from the topic to make sure the performance producer has been created successfully.

Enable client memory limit controller by default

a754347

shoothzj approved these changes Dec 15, 2021

View reviewed changes

merlimat requested changes Dec 15, 2021

View reviewed changes

merlimat added this to the 2.10.0 milestone Dec 15, 2021

merlimat added the type/new-default label Dec 15, 2021

github-actions bot assigned HQebupt Dec 15, 2021

github-actions bot added the doc-label-missing label Dec 15, 2021

liangyuanpeng approved these changes Dec 16, 2021

View reviewed changes

HQebupt force-pushed the pip-120 branch from 6e46ca9 to 94bb924 Compare December 16, 2021 06:29

Anonymitaet added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-label-missing labels Dec 16, 2021

Anonymitaet mentioned this pull request Dec 16, 2021

PIP-118: reconnect broker when ZooKeeper session expires #13341

Merged

2 tasks

HQebupt force-pushed the pip-120 branch from 94bb924 to ceed450 Compare December 17, 2021 13:30

HQebupt requested a review from merlimat December 18, 2021 07:07

HQebupt force-pushed the pip-120 branch 2 times, most recently from 2874be4 to 2db2264 Compare December 23, 2021 12:33

Deprecate maxPendingMessagesAcrossPartitions

f4e6d2d

HQebupt force-pushed the pip-120 branch from 2db2264 to f4e6d2d Compare December 23, 2021 12:35

merlimat approved these changes Jan 22, 2022

View reviewed changes

merlimat merged commit 11bfc0e into apache:master Jan 22, 2022

merlimat added the release/note-required label Jan 22, 2022

codelipenghui mentioned this pull request Apr 24, 2022

[fix][tools] Only apply maxPendingMessagesAcrossPartitions if it presents #15283

Merged

4 tasks

MMirelli mentioned this pull request May 20, 2022

[Broker] Support configuration of the memory allocated to replication producer #15691

Closed

sijie mentioned this pull request May 20, 2022

ISSUE-15691: [Broker] Support configuration of the memory allocated to replication producer streamnative/pulsar-archived#4249

Open

This was referenced May 24, 2022

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

Merged

[Broker, Functions, Websocket] Disable memory limit controller in internal Pulsar clients #15752

Merged

aymkhalil mentioned this pull request Apr 11, 2023

Update pulsar dependency to 2.10.3 datastax/cdc-apache-cassandra#144

Merged

shibd mentioned this pull request Nov 12, 2024

[fix][client] The partitionedProducer maxPendingMessages always is 0 #23593

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIP-120 : Enable client memory limit controller by default #13344

PIP-120 : Enable client memory limit controller by default #13344

HQebupt commented Dec 15, 2021 •

edited

Loading

merlimat left a comment

merlimat Dec 15, 2021

HQebupt Dec 16, 2021

merlimat Dec 23, 2021

HQebupt Dec 23, 2021

github-actions bot commented Dec 15, 2021

shoothzj commented Dec 16, 2021

HQebupt commented Dec 16, 2021

HQebupt commented Dec 16, 2021 •

edited

Loading

HQebupt commented Dec 16, 2021

Anonymitaet commented Dec 16, 2021

HQebupt commented Dec 16, 2021

HQebupt commented Dec 16, 2021

HQebupt commented Dec 18, 2021

yangl commented Dec 23, 2021

merlimat commented Dec 23, 2021

HQebupt commented Dec 23, 2021

HQebupt commented Dec 24, 2021

HQebupt commented Jan 22, 2022

PIP-120 : Enable client memory limit controller by default #13344

PIP-120 : Enable client memory limit controller by default #13344

Conversation

HQebupt commented Dec 15, 2021 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

merlimat left a comment

Choose a reason for hiding this comment

merlimat Dec 15, 2021

Choose a reason for hiding this comment

HQebupt Dec 16, 2021

Choose a reason for hiding this comment

merlimat Dec 23, 2021

Choose a reason for hiding this comment

HQebupt Dec 23, 2021

Choose a reason for hiding this comment

github-actions bot commented Dec 15, 2021

shoothzj commented Dec 16, 2021

HQebupt commented Dec 16, 2021

HQebupt commented Dec 16, 2021 • edited Loading

HQebupt commented Dec 16, 2021

Anonymitaet commented Dec 16, 2021

HQebupt commented Dec 16, 2021

HQebupt commented Dec 16, 2021

HQebupt commented Dec 18, 2021

yangl commented Dec 23, 2021

merlimat commented Dec 23, 2021

HQebupt commented Dec 23, 2021

HQebupt commented Dec 24, 2021

HQebupt commented Jan 22, 2022

HQebupt commented Dec 15, 2021 •

edited

Loading

HQebupt commented Dec 16, 2021 •

edited

Loading