Skip to content

Conversation

@thetumbled
Copy link
Member

@thetumbled thetumbled commented Jun 3, 2024

Motivation

There are two error in method org.apache.pulsar.broker.loadbalance.impl.ModularLoadManagerImpl#createLoadSheddingStrategy.

  • log LoadSheddingStrategy instead of PlacementStrategy.
  • create ThresholdShedder by default instead of OverloadShedder.

Modifications

Fix these two errors.
Fail fast when it failed to create LoadSheddingStrategy.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

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:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 3, 2024
@thetumbled thetumbled force-pushed the Fix_CreateLoadSheddingStrategy branch from 5cbfd8d to 5cf83d1 Compare June 3, 2024 12:19
@thetumbled
Copy link
Member Author

PTAL, thanks. @lhotari @Demogorgon314 @heesung-sn @BewareMyPower

@thetumbled
Copy link
Member Author

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 73.24%. Comparing base (bbc6224) to head (5cf83d1).
Report is 369 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22827      +/-   ##
============================================
- Coverage     73.57%   73.24%   -0.34%     
- Complexity    32624    32638      +14     
============================================
  Files          1877     1889      +12     
  Lines        139502   141744    +2242     
  Branches      15299    15554     +255     
============================================
+ Hits         102638   103816    +1178     
- Misses        28908    29908    +1000     
- Partials       7956     8020      +64     
Flag Coverage Δ
inttests 27.44% <0.00%> (+2.85%) ⬆️
systests 24.67% <0.00%> (+0.35%) ⬆️
unittests 72.26% <0.00%> (-0.58%) ⬇️

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

Files Coverage Δ
...roker/loadbalance/impl/ModularLoadManagerImpl.java 84.20% <0.00%> (-0.79%) ⬇️

... and 373 files with indirect coverage changes

@BewareMyPower BewareMyPower changed the title [fix] [broker] fix create LoadSheddingStrategy instance. [improve] [broker] Fail fast when it failed to create LoadSheddingStrategy instance Jun 11, 2024
@thetumbled
Copy link
Member Author

/pulsarbot rerun-failure-checks

@BewareMyPower BewareMyPower merged commit 1770cbc into apache:master Jun 12, 2024
nodece pushed a commit to nodece/pulsar that referenced this pull request Jul 23, 2024
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Jul 24, 2024
@lhotari lhotari added this to the 4.0.0 milestone Oct 14, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants