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

[pulsar-broker] Support configuration to rate-limit dispatching on batch message #12294

Merged
merged 1 commit into from
Oct 20, 2021

Conversation

rdhabalia
Copy link
Contributor

Motivation

We introduced dispatch throttling as part of PIP-3 which helps brokers to control fan-out and prevent broker's instability due to large message dispatch. However, current throttling applies rate-limiting based on a number of messages within batch-message. Because of that overall, broker dispatches less number of messages even though the broker has enough bandwidth if the given batch message has a large number of messages eg: 100 or 1000 msgs per batch. Also, various topics under a namespace have messages with various batch sizes and it makes unpredictable throttling for all topics under a namespace and it causes backlog building for some of the topics.
Therefore, the broker should have a configuration to count a number of batch messages rather than total messages within each batch message during rate limiting.

Modification

Add configuration dispatchThrottlingOnBatchMessageEnabled to support rate-limiting dispatching on the batch messages rather than individual messages within batch messages. By default it will be disabled so, it won't change the existing behavior of the broker.

Result

Avoid backlog building for topics in a namespace with different sizes of batch messages during dispatch rate-limiting.

@rdhabalia rdhabalia added area/broker doc-required Your PR changes impact docs and you will update later. labels Oct 7, 2021
@rdhabalia rdhabalia added this to the 2.9.0 milestone Oct 7, 2021
@rdhabalia rdhabalia self-assigned this Oct 7, 2021
@Anonymitaet
Copy link
Member

@rdhabalia feel free to ping me if you need a doc review.

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

but I am not sure it is good to add this to 2.9.0, as I see no discussion on this feature.

I am fine to merge it for 2.10.0

@eolivelli eolivelli modified the milestones: 2.9.0, 2.10.0 Oct 13, 2021
@rdhabalia
Copy link
Contributor Author

this change doesn't change default behavior because it comes with the config. also it's addressing serious issue for batch messages so, I don't see any issue for merging for 2.9

@rdhabalia rdhabalia merged commit 5523604 into apache:master Oct 20, 2021
wangjialing218 pushed a commit to wangjialing218/pulsar that referenced this pull request Oct 21, 2021
@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Nov 25, 2021
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-complete Your PR changes impact docs and the related docs have been already added.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants