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

[improve][broker] PIP-364: Introduce a new load balance algorithm AvgShedder #22949

Merged
merged 31 commits into from
Jul 11, 2024

Conversation

thetumbled
Copy link
Member

@thetumbled thetumbled commented Jun 20, 2024

Motivation

Implementation PR for PIP: #22946.

Modifications

  • Introduce new algo: AvgShedder.
  • provide a way to bind shedding strategy and placement strategy.

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:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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#59

Copy link

@thetumbled Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@thetumbled
Copy link
Member Author

As the proposal PR has been merged, we can proceed with the implementation PR, thanks. @lhotari @heesung-sn @Demogorgon314 @BewareMyPower @poorbarcode

thetumbled and others added 3 commits July 5, 2024 18:01
…ce/impl/AvgShedder.java

Co-authored-by: Kai Wang <kwang@streamnative.io>
…ce/impl/AvgShedder.java

Co-authored-by: Kai Wang <kwang@streamnative.io>
…ce/impl/AvgShedder.java

Co-authored-by: Kai Wang <kwang@streamnative.io>
thetumbled and others added 3 commits July 8, 2024 16:47
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

Besides, the findBundlesForUnloading method is too long (about 200 lines), which is hard to read. It's worth splitting it into multiple private methods. From what I see, there should be 3 steps:

  1. loadData.getBrokerData() -> updated brokerScoreMap (Map<String, Double>) and its sorted keys according to the value comparation
  2. Update brokerHitCountForLow and brokerHitCountForHigh according to the threshold related configs.
  3. Update and return selectedBundlesCache according to loadData.getRecentlyUnloadedBundles() and the two maps above.

@thetumbled
Copy link
Member Author

Besides, the findBundlesForUnloading method is too long (about 200 lines), which is hard to read. It's worth splitting it into multiple private methods. From what I see, there should be 3 steps:

  1. loadData.getBrokerData() -> updated brokerScoreMap (Map<String, Double>) and its sorted keys according to the value comparation
  2. Update brokerHitCountForLow and brokerHitCountForHigh according to the threshold related configs.
  3. Update and return selectedBundlesCache according to loadData.getRecentlyUnloadedBundles() and the two maps above.

Updated, thanks for review!

Copy link
Member

@Demogorgon314 Demogorgon314 left a comment

Choose a reason for hiding this comment

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

Good job! Can you add the AvgShedder algorithm for ExtensibleLoadManager.

@thetumbled
Copy link
Member Author

Good job! Can you add the AvgShedder algorithm for ExtensibleLoadManager.

Sounds good, i will try to acheive that.

@thetumbled
Copy link
Member Author

PIP-364: Introduce a new load balance algorithm AvgShedder

@thetumbled thetumbled merged commit c160cc9 into apache:master Jul 11, 2024
56 of 57 checks passed
@Demogorgon314
Copy link
Member

Hi @thetumbled, can you help send a discussion to cherry-pick this new algorithm into branch-3.0, branch-3.2, and branch-3.3?

@thetumbled
Copy link
Member Author

Hi @thetumbled, can you help send a discussion to cherry-pick this new algorithm into branch-3.0, branch-3.2, and branch-3.3?

discussion link: https://lists.apache.org/thread/9yqq8wky1k73hqz6xhvstk6x0ycknqgt

@thetumbled thetumbled modified the milestone: 3.0 Jul 19, 2024
nodece pushed a commit to nodece/pulsar that referenced this pull request Jul 23, 2024
…edder (apache#22949)

Co-authored-by: Kai Wang <kwang@streamnative.io>
Co-authored-by: Yunze Xu <xyzinfernity@163.com>

(cherry picked from commit c160cc9)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Jul 24, 2024
…edder (apache#22949)

Co-authored-by: Kai Wang <kwang@streamnative.io>
Co-authored-by: Yunze Xu <xyzinfernity@163.com>

(cherry picked from commit c160cc9)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@thetumbled
Copy link
Member Author

Tip: If you want to cherry pick this pr, please cherry pick this patch too:
#23156

@lhotari lhotari added this to the 4.0.0 milestone Oct 14, 2024
@lhotari lhotari changed the title [improve] [pip] PIP-364: Introduce a new load balance algorithm AvgShedder [improve][broker] PIP-364: Introduce a new load balance algorithm AvgShedder Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required Your PR changes impact docs and you will update later. ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants