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

[do not merge] [improve] [broker] Make the dispatch rate limit more precise when subscription is created #18581

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Nov 23, 2022

This PR should merge after #18553.

Motivation

The Dispatch rate limiter serves two purposes:

  1. Limit your consumption speed
  2. When there are many backlogs, bk can be protected to avoid excessive I/O

But the current design has the following problems:

available permits indicates how many messages the client needs, and then uses the rate limiter to determine how many entries to read. If batch production is not enabled, it is working. When batch production is enabled, the rate limiter counts the number of entries as the number of messages, resulting in excessive entries to be read.

In PR #6719, preciseDispatcherFlowControl was added to optimize the above problem: If enabled preciseDispatcherFlowControl, we call availablePermits / avgMessagesPerEntry to calculate how many entries to read.

After PR #18553, there are still such scenarios: failing to get avgMessagesPerEntry resulting in reading excessive messages:

  1. Create a new topic and a new subscription, since no data has been consumed yet, the consumer does not know how many messages per entry.
  2. Use the created topic, only one subscription, and use the failover mode for consumption. Every time a new consumer becomes an active consumer, can not get avgMessagesPerEntry.

Modifications

  • Record avgMessagesPerEntry on the topic, used if avgMessagesPerEntry could not be found in subscriptions.
  • If avgMessagesPerEntry is not initialized (it could also be called the first reading), at most one message can be read. Just like the original design: only one message is read when ledger.getStats().getEntrySizeAverage() is not initialized.
    • Why not use mechanism ledger.getStats().getEntrySizeAverage() instead of creating another attribute topic.avgMessagesPerEntry? Because ledger stat is not set immediately after produce messages, need to wait for the scheduled task(default interval 60 seconds) in ManagedLedgerFactoryImpl.
    • See the code below (line-3396):

private int applyMaxSizeCap(int maxEntries, long maxSizeBytes) {
if (maxSizeBytes == NO_MAX_SIZE_LIMIT) {
return maxEntries;
}
double avgEntrySize = ledger.getStats().getEntrySizeAverage();
if (!Double.isFinite(avgEntrySize)) {
// We don't have yet any stats on the topic entries. Let's try to use the cursor avg size stats
avgEntrySize = (double) entriesReadSize / (double) entriesReadCount;
}

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 23, 2022
@poorbarcode poorbarcode force-pushed the improve/topic_stat_msg_count_per_entry branch from 66b4c1b to c06290d Compare November 23, 2022 16:01
@poorbarcode poorbarcode force-pushed the improve/topic_stat_msg_count_per_entry branch from c06290d to 55f3530 Compare November 23, 2022 16:33
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Jan 5, 2023
@Technoboy- Technoboy- added this to the 3.2.0 milestone Jul 31, 2023
@Technoboy- Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023
@coderzc coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024
@lhotari
Copy link
Member

lhotari commented Oct 14, 2024

please rebase

@lhotari lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024
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 Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants