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

[PIP-74] Support consumer client memory limit #15216

Merged
merged 1 commit into from
May 7, 2022

Conversation

Jason918
Copy link
Contributor

@Jason918 Jason918 commented Apr 19, 2022

Motivation

This is the final part of PIP-74.

https://github.com/apache/pulsar/wiki/PIP-74%3A-Pulsar-client-memory-limits

Modifications

  1. This feature does not take effect by default for now. Consumer memory usage take account only if autoScaledReceiverQueueSizeEnabled is enabled. We can change this to default when this feature is verified by more production usage.
  2. Block consumer receiver queue size expansion if memory usage > 75%
  3. Trigger consumer receiver queue size shrinking if memory usage > 95%

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change added tests and can be verified as follows:

  • ConsumerMemoryLimitTest
  • MemoryLimitControllerTest

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?

  • no-need-doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 19, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Apr 19, 2022
@codelipenghui codelipenghui added type/feature The PR added a new feature or issue requested a new feature component/client-java labels Apr 19, 2022
@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to introduce a publish/consume memory ratio? In most cases, users are consuming messages from the consumer and publish new messages, and ack the consumed messages. I'm worried about if the consumer can use up the memory, the producer not able to publish, so that the consumer queue can't be cleanup, it looks like a deadlock.

@Jason918
Copy link
Contributor Author

Do we need to introduce a publish/consume memory ratio? In most cases, users are consuming messages from the consumer and publish new messages, and ack the consumed messages. I'm worried about if the consumer can use up the memory, the producer not able to publish, so that the consumer queue can't be cleanup, it looks like a deadlock.

Yes, good point. Sharing memory quota does have this kind of isolation problem. Not only between producers and consumers, but also between different consumers. E.g. one consumer use up the memory, other consumers may not be able to receive any message any more.

Currently, I suggest to enable this feature within a new client instance.

I am not sure it's the right time to just split publish/consume memory quota, by ratio or other ways. One alternative is to reduce local message buffer directly and make broker resend message later. This may require some change in protocols. Just a brief thought yet. Let's dig deeper in anther PIP(like solving isolation problem when sharing producers and consumers in one client instance).

@codelipenghui

@AnonHxy
Copy link
Contributor

AnonHxy commented May 5, 2022

LGTM

Copy link
Contributor

@gaozhangmin gaozhangmin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. left one comment.

@Gleiphir2769
Copy link
Contributor

Do we need to introduce a publish/consume memory ratio? In most cases, users are consuming messages from the consumer and publish new messages, and ack the consumed messages. I'm worried about if the consumer can use up the memory, the producer not able to publish, so that the consumer queue can't be cleanup, it looks like a deadlock.

Yes, good point. Sharing memory quota does have this kind of isolation problem. Not only between producers and consumers, but also between different consumers. E.g. one consumer use up the memory, other consumers may not be able to receive any message any more.

Currently, I suggest to enable this feature within a new client instance.

I am not sure it's the right time to just split publish/consume memory quota, by ratio or other ways. One alternative is to reduce local message buffer directly and make broker resend message later. This may require some change in protocols. Just a brief thought yet. Let's dig deeper in anther PIP(like solving isolation problem when sharing producers and consumers in one client instance).

@codelipenghui

Hi @Jason918, I find this discussion from apache/pulsar-client-go#927. Just some question I want to figure out.
Why not let the producer/consumer have their own MemoryLimitController? And split the memoryLimit setting into consumerMemoryLimit and producerMemoryLimit. Are there any other considerations?

@michaeljmarshall
Copy link
Member

Great question @Gleiphir2769. If you don't get an answer, it might help to post this on the pulsar dev mailing list.

@Gleiphir2769
Copy link
Contributor

Great question @Gleiphir2769. If you don't get an answer, it might help to post this on the pulsar dev mailing list.

Good suggestion @michaeljmarshall ! I will start a discussion on the dev mailing list.

@Jason918
Copy link
Contributor Author

Why not let the producer/consumer have their own MemoryLimitController? And split the memoryLimit setting into consumerMemoryLimit and producerMemoryLimit. Are there any other considerations?

@Gleiphir2769 I have replied in the mail list. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants