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-307: Add feature flag config option #21866

Merged
merged 12 commits into from
Jan 11, 2024

Conversation

dragosvictor
Copy link
Contributor

@dragosvictor dragosvictor commented Jan 9, 2024

PIP: PIP-307

Motivation

This PR adds a non-dynamic option loadBalancerMultiPhaseBundleUnload that allows administrators to enable/disable the graceful producer/consumer shutdown upon bundle unloading (PIP-307). Without this, the only means to disable the new mechanism would be disabling the new Extensible Load Manager altogether, which might otherwise work as expected.

Modifications

Adds configuration option loadBalancerMultiPhaseBundleUnload with the default value set to true, enabling the mechanism described in PIP-307 out-of-the-box. With the flag disabled, the clients are disconnected immediately on the source broker upon reaching the RELEASE state in the service channel. No lookup information is forwarded to the clients, causing them to fall back on the old behavior, wherein they have to issue lookup calls to locate the new broker serving the topic.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added unit test org.apache.pulsar.broker.loadbalance.extensions.ExtensibleLoadManagerImplTest#testOptimizeUnloadDisable that verifies client lookups occur on both producers and consumers when the flag is set to disabled
  • Manually verified enablement/disablement of flag in a k8s deployment

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: dragosvictor#5

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Jan 9, 2024
@dragosvictor dragosvictor marked this pull request as ready for review January 9, 2024 00:11
@Technoboy- Technoboy- added this to the 3.3.0 milestone Jan 11, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (cea5c93) 73.59% compared to head (f39a066) 73.59%.
Report is 11 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #21866      +/-   ##
============================================
- Coverage     73.59%   73.59%   -0.01%     
- Complexity    32323    32340      +17     
============================================
  Files          1858     1859       +1     
  Lines        138174   138291     +117     
  Branches      15148    15161      +13     
============================================
+ Hits         101696   101776      +80     
- Misses        28608    28626      +18     
- Partials       7870     7889      +19     
Flag Coverage Δ
inttests 24.12% <4.41%> (-0.13%) ⬇️
systests 23.71% <16.17%> (-0.08%) ⬇️
unittests 72.88% <70.58%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java 99.39% <100.00%> (+<0.01%) ⬆️
...dbalance/extensions/ExtensibleLoadManagerImpl.java 79.51% <100.00%> (-0.04%) ⬇️
...ervice/AbstractDispatcherSingleActiveConsumer.java 90.55% <100.00%> (ø)
...va/org/apache/pulsar/broker/service/ServerCnx.java 72.08% <100.00%> (-0.32%) ⬇️
...rg/apache/pulsar/client/impl/TopicListWatcher.java 67.14% <100.00%> (+0.47%) ⬆️
...xtensions/channel/ServiceUnitStateChannelImpl.java 84.45% <88.88%> (-0.01%) ⬇️
...rg/apache/pulsar/broker/service/AbstractTopic.java 87.71% <0.00%> (-0.30%) ⬇️
...ar/client/impl/PatternMultiTopicsConsumerImpl.java 83.45% <63.26%> (-4.51%) ⬇️

... and 80 files with indirect coverage changes

@heesung-sn heesung-sn merged commit c834feb into apache:master Jan 11, 2024
@dragosvictor dragosvictor deleted the pip-307-feature-flag branch January 31, 2024 19:46
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.

6 participants