Skip to content

[improve] [broker] Follower do not need to retrieve load data in zk.#20970

Open
thetumbled wants to merge 4 commits into
apache:masterfrom
thetumbled:Feature_OptimizeZkLatency
Open

[improve] [broker] Follower do not need to retrieve load data in zk.#20970
thetumbled wants to merge 4 commits into
apache:masterfrom
thetumbled:Feature_OptimizeZkLatency

Conversation

@thetumbled

@thetumbled thetumbled commented Aug 10, 2023

Copy link
Copy Markdown
Member

Motivation

Load balance module in pulsar broker pose greate pressure on zk.
image
image

Currently, ModularLoadManagerImpl is the main LoadManager in 2.x version, which work as a centralized way. Most of the time, leader broker take the responsibility of shedding and assignning, while followers update their load data to zk for leader. Leader broker retrieve load data of all brokers in the cluster to make decision.
But, i found that not only leader will retrieve all load data (include LocalBrokerData and BundleData), but also followers, which is unnecessary and pose greate pressure to zk.

Modifications

Disable load data retrieve for followers.

Result as follows, only the zk instance that serve for leader broker present high throught.
image
The peak total read traffic of the zk cluster has decreased from 24kb/s to 10kb/s.
image

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)
This change is already covered by existing tests, such as (please describe tests).

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: thetumbled#23

@lifepuzzlefun

Copy link
Copy Markdown
Contributor

I support the idea, in large cluster deployment follower read zk is very heavy. We need more unit test on failover or other logic if we remove the retrieve logic.

@thetumbled

Copy link
Copy Markdown
Member Author

PTAL, thanks. @codelipenghui @heesung-sn @Demogorgon314

@heesung-sohn

Copy link
Copy Markdown
Contributor

But, i found that not only leader will retrieve all load data (include LocalBrokerData and BundleData), but also followers, which is unnecessary and pose greate pressure to zk.

Can we confirm that followers never use any of this info in their logic(any regressions caused by this change)?

// Executor is shutting down
if (isLeader()) {
try {
scheduler.submit(ModularLoadManagerImpl.this::updateAll);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We need a unit test to verify this change.

@github-actions

Copy link
Copy Markdown

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

@github-actions github-actions Bot added the Stale label Sep 16, 2023
@Technoboy- Technoboy- added this to the 3.3.0 milestone Dec 22, 2023
@coderzc coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024
@lhotari lhotari modified the milestones: 4.0.0, 4.1.0 Oct 11, 2024
@coderzc coderzc modified the milestones: 4.1.0, 4.2.0 Sep 1, 2025
@lhotari lhotari modified the milestones: 4.2.0, 4.3.0 Apr 27, 2026
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.

7 participants