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.

Sign up for GitHub

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

Jump to bottom

[Broker] Disable memory limit controller for broker client and replication clients #15723

Merged
merged 1 commit into from
May 25, 2022
Merged

[Broker] Disable memory limit controller for broker client and replication clients #15723

merged 1 commit into from
May 25, 2022

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented May 23, 2022 •
edited
Loading

Fixes #15691

Motivation

There's a performance regression in 2.10 in geo-replication since the client memory limit is enabled by default in "PIP-120: Enable client memory limit by default", #13306 .

Modifications

  • disable memory limit by default for broker client and replication clients
  • restore the default maxPendingMessages & maxPendingMessagesAcrossPartitions limits for producers to the defaults used before PIP-120 changes when the memory limit is disabled.
    • The broker client's producer is used in a few location and it would be more consistent also for other applications when the defaults are adjusted if the memory limit is disabled.

@lhotari lhotari added type/bug The PR fixed a bug or issue reported a bug release/2.10.1 labels May 23, 2022
@lhotari lhotari added this to the 2.11.0 milestone May 23, 2022
@lhotari lhotari self-assigned this May 23, 2022
@github-actions
Copy link

@lhotari: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)

@lhotari lhotari mentioned this pull request May 23, 2022
Closed
@lhotari lhotari mentioned this pull request May 23, 2022
Closed
@lhotari lhotari added the doc-not-needed Your PR changes do not impact docs label May 23, 2022
@github-actions
Copy link

@lhotari:Thanks for providing doc info!

@lhotari
Copy link
Member Author

lhotari commented May 23, 2022

I also added #15724 to address a problem seen in #15691

merlimat
merlimat reviewed May 23, 2022
View reviewed changes
conf/broker.conf Outdated Show resolved Hide resolved
michaeljmarshall
michaeljmarshall requested changes May 23, 2022
View reviewed changes
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

It looks like this is only a partial solution. We should configure all clients with the correct configuration.

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Outdated Show resolved Hide resolved
@lhotari
Copy link
Member Author

lhotari commented May 24, 2022 •
edited
Loading

It looks like this is only a partial solution. We should configure all clients with the correct configuration.

@michaeljmarshall It's intentional that this PR addresses only the replication clients. That's the problem that we have faced.

@michaeljmarshall
Copy link
Member

It looks like this is only a partial solution. We should configure all clients with the correct configuration.

@michaeljmarshall It's intentional that this PR addresses only the replication clients. That's the problem that we have faced.

@lhotari - sorry, I completely missed the configuration name. Do you think it'll make sense long term to have this separate config? I have been thinking about consolidating client configuration in to simplify broker client configuration.

@lhotari
Copy link
Member Author

lhotari commented May 24, 2022 •
edited
Loading

@lhotari - sorry, I completely missed the configuration name. Do you think it'll make sense long term to have this separate config? I have been thinking about consolidating client configuration in to simplify broker client configuration.

@michaeljmarshall Good point. After all it might be worth hard coding the setting to 0 for the actual broker client + all replication clients as @merlimat suggested. The benefit of this would be that we don't unnecessarily introduce new settings which aren't useful in the end. I will revisit this PR.

We can add the way to configure the limits later.

@lhotari lhotari marked this pull request as draft May 24, 2022 04:43
@lhotari lhotari changed the title [Broker] Add replicationClientMemoryLimitBytes setting for configuring replication client memory limit [Broker] Disable memory limit controller for broker client and replication clients May 24, 2022
@lhotari lhotari force-pushed the lh-replication-client-memory-limit branch from 6fb1287 to 3c277ce Compare May 24, 2022 07:57
@lhotari lhotari marked this pull request as ready for review May 24, 2022 08:02
@lhotari
[Broker] Disable memory limit controller for broker client and replic…
61d4a8c 
…ation clients

- disable memory limit by default for broker client and replication clients
- restore maxPendingMessages and maxPendingMessagesAcrossPartitions when memory limit is
  disabled so that pre-PIP-120 default configuration is restored when limit is disabled
@lhotari lhotari force-pushed the lh-replication-client-memory-limit branch from 3c277ce to 61d4a8c Compare May 24, 2022 08:03
@lhotari lhotari requested a review from nicoloboschi May 24, 2022 08:04
eolivelli
eolivelli approved these changes May 24, 2022
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

we should add this to branch-2.10

This was referenced May 24, 2022
Merged
Merged
lhotari added a commit to datastax/pulsar that referenced this pull request May 24, 2022
@lhotari
[Broker] Disable memory limit controller for broker client and replic…
4a2f75e 
…ation clients (apache#15723)

- disable memory limit by default for broker client and replication clients
- restore maxPendingMessages and maxPendingMessagesAcrossPartitions when memory limit is
  disabled so that pre-PIP-120 default configuration is restored when limit is disabled
@lhotari
Copy link
Member Author

lhotari commented May 24, 2022

/pulsarbot rerun-failure-checks

@merlimat merlimat merged commit 563a7cb into apache:master May 25, 2022
merlimat pushed a commit that referenced this pull request May 25, 2022
@lhotari @merlimat
[Broker] Disable memory limit controller for broker client and replic…
a56f041 
…ation clients (#15723)

- disable memory limit by default for broker client and replication clients
- restore maxPendingMessages and maxPendingMessagesAcrossPartitions when memory limit is
  disabled so that pre-PIP-120 default configuration is restored when limit is disabled
lhotari added a commit that referenced this pull request May 25, 2022
@lhotari
Fix cherry-picking issue with #15723
d2f0346 
- this method was missing from a56f041
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes

@eolivelli eolivelli eolivelli approved these changes

@HQebupt HQebupt HQebupt approved these changes

@merlimat merlimat merlimat approved these changes

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@nicoloboschi nicoloboschi Awaiting requested review from nicoloboschi

Assignees

@lhotari lhotari

Labels
cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.10.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.

[Broker] Support configuration of the memory allocated to replication producer
5 participants
@lhotari @michaeljmarshall @merlimat @HQebupt @eolivelli