Skip to content

[fix][broker] Update TransferShedder underloaded broker check to consider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check#22321

Merged
heesung-sohn merged 1 commit into
apache:masterfrom
heesung-sohn:transfer-shedder-underloaded-cond
Apr 3, 2024

Conversation

@heesung-sohn

@heesung-sohn heesung-sohn commented Mar 21, 2024

Copy link
Copy Markdown
Contributor

Motivation

Checking zero traffic is not practical to identify if a broker is too underloaded or not(when first joining the cluster). There might be some health-checking traffic, which might make the broker's traffic slightly bigger than 1 byte/s. We better consider some relative measurements to check if a broker is significantly underloaded.

Modifications

  • Update TransferShedder underloaded broker check to consider max loaded broker's msgThroughputEMA
  • Delete isLoadManagerExtensionEnabled(ServiceConfiguration conf) to use only isLoadManagerExtensionEnabled(PulsarService pulsar) func

Verifying this change

  • Make sure that the change passes the CI checks.

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 Mar 21, 2024
@heesung-sohn heesung-sohn self-assigned this Mar 21, 2024

@Demogorgon314 Demogorgon314 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@dragosvictor dragosvictor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@codecov-commenter

codecov-commenter commented Mar 25, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 85.29412% with 5 lines in your changes missing coverage. Please review.

Project coverage is 73.80%. Comparing base (bbc6224) to head (1105476).
Report is 582 commits behind head on master.

Files with missing lines Patch % Lines
...dbalance/extensions/scheduler/TransferShedder.java 80.00% 1 Missing and 1 partial ⚠️
...ache/pulsar/broker/namespace/NamespaceService.java 86.66% 1 Missing and 1 partial ⚠️
...n/java/org/apache/pulsar/broker/PulsarService.java 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22321      +/-   ##
============================================
+ Coverage     73.57%   73.80%   +0.22%     
+ Complexity    32624    32418     -206     
============================================
  Files          1877     1885       +8     
  Lines        139502   139881     +379     
  Branches      15299    15321      +22     
============================================
+ Hits         102638   103235     +597     
+ Misses        28908    28664     -244     
- Partials       7956     7982      +26     
Flag Coverage Δ
inttests 26.96% <50.00%> (+2.37%) ⬆️
systests 24.58% <0.00%> (+0.25%) ⬆️
unittests 73.07% <85.29%> (+0.23%) ⬆️

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

Files with missing lines Coverage Δ
...pache/pulsar/broker/admin/impl/NamespacesBase.java 74.63% <100.00%> (+1.55%) ⬆️
...dbalance/extensions/ExtensibleLoadManagerImpl.java 81.72% <100.00%> (+1.64%) ⬆️
...rg/apache/pulsar/broker/web/PulsarWebResource.java 64.44% <100.00%> (+0.41%) ⬆️
...n/java/org/apache/pulsar/broker/PulsarService.java 83.60% <75.00%> (+1.23%) ⬆️
...dbalance/extensions/scheduler/TransferShedder.java 81.86% <80.00%> (-1.06%) ⬇️
...ache/pulsar/broker/namespace/NamespaceService.java 73.00% <86.66%> (+0.76%) ⬆️

... and 181 files with indirect coverage changes

@heesung-sohn heesung-sohn force-pushed the transfer-shedder-underloaded-cond branch from 9eb7318 to f4ab491 Compare April 1, 2024 16:39
@heesung-sohn

Copy link
Copy Markdown
Contributor Author

Hi, @mattisonchao I saw you merged this PR , #22379. Can we also merge this too? It appears that CI is broken now?

@heesung-sohn heesung-sohn deleted the transfer-shedder-underloaded-cond branch April 2, 2024 17:43
@heesung-sohn heesung-sohn restored the transfer-shedder-underloaded-cond branch April 2, 2024 22:59
@heesung-sohn heesung-sohn reopened this Apr 2, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check
@heesung-sohn heesung-sohn force-pushed the transfer-shedder-underloaded-cond branch from f4ab491 to 1105476 Compare April 2, 2024 23:01
@heesung-sohn heesung-sohn merged commit d7d5452 into apache:master Apr 3, 2024
@heesung-sohn heesung-sohn deleted the transfer-shedder-underloaded-cond branch April 3, 2024 15:10
heesung-sohn added a commit to heesung-sohn/pulsar that referenced this pull request Apr 3, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check (apache#22321)

(cherry picked from commit d7d5452)
heesung-sohn added a commit to heesung-sohn/pulsar that referenced this pull request Apr 3, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check (apache#22321)

(cherry picked from commit d7d5452)
heesung-sohn added a commit to heesung-sohn/pulsar that referenced this pull request Apr 3, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check (apache#22321)

(cherry picked from commit d7d5452)
heesung-sohn added a commit to heesung-sohn/pulsar that referenced this pull request Apr 3, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check (apache#22321)

(cherry picked from commit d7d5452)
heesung-sohn added a commit that referenced this pull request Apr 4, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check (#22321) (#22416)
heesung-sohn added a commit that referenced this pull request Apr 4, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check (#22321) (#22417)
heesung-sohn added a commit that referenced this pull request Apr 4, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check (#22321) (#22418)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 5, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check (apache#22321) (apache#22417)

(cherry picked from commit 651908a)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 8, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check (apache#22321) (apache#22417)

(cherry picked from commit 651908a)
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check (apache#22321)
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