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

[fix][broker] Fix AvgShedder strategy check #23156

Conversation

Demogorgon314
Copy link
Member

@Demogorgon314 Demogorgon314 commented Aug 12, 2024

Motivation

The check for AvgShedder strategy is wrong, we should use loadBalancerLoadPlacementStrategy instead of loadBalancerPlacementStrategy.

The loadBalancerPlacementStrategy is used by SimpleLoadManagerImpl, and it is deprecated.

Modifications

Use loadBalancerLoadPlacementStrategy instead of loadBalancerPlacementStrategy to check the AvgShedder strategy

Documentation

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

@Demogorgon314 Demogorgon314 self-assigned this Aug 12, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 12, 2024
Copy link
Contributor

@heesung-sn heesung-sn left a comment

Choose a reason for hiding this comment

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

Do you know Why we have these two similar configs? This is confusing indeed.

@Demogorgon314
Copy link
Member Author

@heesung-sn The loadBalancerPlacementStrategy is used by SimpleLoadManagerImpl, and it is deprecated.

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2024

Codecov Report

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

Project coverage is 74.53%. Comparing base (bbc6224) to head (d699090).
Report is 780 commits behind head on master.

Files with missing lines Patch % Lines
...roker/loadbalance/impl/ModularLoadManagerImpl.java 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23156      +/-   ##
============================================
+ Coverage     73.57%   74.53%   +0.96%     
- Complexity    32624    34102    +1478     
============================================
  Files          1877     1919      +42     
  Lines        139502   144269    +4767     
  Branches      15299    15778     +479     
============================================
+ Hits         102638   107533    +4895     
+ Misses        28908    28502     -406     
- Partials       7956     8234     +278     
Flag Coverage Δ
inttests 27.62% <0.00%> (+3.04%) ⬆️
systests 24.79% <0.00%> (+0.47%) ⬆️
unittests 73.87% <0.00%> (+1.03%) ⬆️

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

Files with missing lines Coverage Δ
...roker/loadbalance/impl/ModularLoadManagerImpl.java 84.32% <0.00%> (-0.67%) ⬇️

... and 488 files with indirect coverage changes

Copy link
Member

@thetumbled thetumbled left a comment

Choose a reason for hiding this comment

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

LGTM.

@thetumbled thetumbled changed the title [improve][broker] Fix AvgShedder strategy check [fix][broker] Fix AvgShedder strategy check Aug 13, 2024
@thetumbled thetumbled merged commit 66cc754 into apache:master Aug 13, 2024
57 checks passed
nodece pushed a commit to nodece/pulsar that referenced this pull request Aug 15, 2024
grssam pushed a commit to grssam/pulsar that referenced this pull request Sep 4, 2024
@thetumbled
Copy link
Member

thetumbled commented Oct 8, 2024

This bug fix is not included into branch 3.0, 3.2 and 3.3, we need to cherry pick it into 3.0 and 3.3 and wait for next release. @heesung-sn @Demogorgon314 @lhotari

thetumbled pushed a commit that referenced this pull request Oct 8, 2024
thetumbled pushed a commit that referenced this pull request Oct 8, 2024
thetumbled pushed a commit that referenced this pull request Oct 8, 2024
@lhotari
Copy link
Member

lhotari commented Oct 8, 2024

This bug fix is not included into branch 3.0, 3.2 and 3.3, we need to cherry pick it into 3.0 and 3.3 and wait for next release. @heesung-sn @Demogorgon314 @lhotari

The person merging the PR should ensure that the release labels are on the PR before it gets merged. That's how the release manager knows what it planned for a release. @thetumbled Please find the information in this email: https://lists.apache.org/thread/ryyfv8o33yyw9hmo5gnxn71boocn3mzs . This information is currently missing from our contribution guide.

@lhotari
Copy link
Member

lhotari commented Oct 8, 2024

@thetumbled The cherry-picking process is now documented in https://pulsar.apache.org/contribute/maintenance-process/

@lhotari lhotari added this to the 4.0.0 milestone Oct 14, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Oct 15, 2024
(cherry picked from commit 66cc754)
(cherry picked from commit 0fa9572)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Oct 16, 2024
(cherry picked from commit 66cc754)
(cherry picked from commit 0fa9572)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants