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] Make BrokerSelectionStrategy pluggable #22553

Conversation

BewareMyPower
Copy link
Contributor

Motivation

When users want to extend the ExtensibleLoadManagerImpl, the BrokerSelectionStrategy cannot be customized, so users have to write much duplicated code for a customized broker selection strategy.

Modifications

Add a stable interface BrokerSelectionStrategyFactory and implements this interface in ExtensibleLoadManagerImpl with the default implementation that returns a LeastResourceUsageWithWeight instance by default. Add CustomBrokerSelectionStrategyTest to show how to customize the broker selection strategy and verify it works.

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:

### Motivation

When users want to extend the `ExtensibleLoadManagerImpl`, the
`BrokerSelectionStrategy` cannot be customized, so users have to write
much duplicated code for a customized broker selection strategy.

### Modifications

Add a stable interface `BrokerSelectionStrategyFactory` and implements
this interface in `ExtensibleLoadManagerImpl` with the default
implementation that returns a `LeastResourceUsageWithWeight` instance by
default. Add `CustomBrokerSelectionStrategyTest` to show how to
customize the broker selection strategy and verify it works.
@BewareMyPower BewareMyPower added this to the 3.3.0 milestone Apr 22, 2024
@BewareMyPower BewareMyPower self-assigned this Apr 22, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 22, 2024
@BewareMyPower
Copy link
Contributor Author

  Error:  Failures: 
  Error:  org.apache.pulsar.broker.loadbalance.extensions.strategy.CustomBrokerSelectionStrategyTest.testSingleBrokerSelected
  [INFO]   Run 1: PASS
  Error:    Run 2: CustomBrokerSelectionStrategyTest.testSingleBrokerSelected:71 expected [34517] but found [38481]

It seems this test is flaky. I will check the failure soon.

@BewareMyPower BewareMyPower marked this pull request as draft April 23, 2024 05:44
@BewareMyPower BewareMyPower marked this pull request as ready for review April 23, 2024 07:07
@BewareMyPower
Copy link
Contributor Author

The test failed because the broker selection strategy of CustomExtensibleLoadManager chooses the broker with the smallest HTTP listen port, while the test uses the PulsarService instance with the smallest binary listen port as the expected broker. I fixed it now

@BewareMyPower BewareMyPower merged commit 89b201e into apache:master Apr 23, 2024
48 of 50 checks passed
@BewareMyPower BewareMyPower deleted the bewaremypower/broker-selection-strategy-config branch April 23, 2024 10:52
Technoboy- pushed a commit to Technoboy-/pulsar that referenced this pull request Apr 24, 2024
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
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 ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants