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] Update TransferShedder underloaded broker check to consider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check #22321

Merged

Conversation

heesung-sn
Copy link
Contributor

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

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-sn heesung-sn self-assigned this Mar 21, 2024
Copy link
Member

@Demogorgon314 Demogorgon314 left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@dragosvictor dragosvictor left a comment

Choose a reason for hiding this comment

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

LGTM!

@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2024

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-sn heesung-sn force-pushed the transfer-shedder-underloaded-cond branch from 9eb7318 to f4ab491 Compare April 1, 2024 16:39
@heesung-sn
Copy link
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-sn heesung-sn closed this Apr 2, 2024
@heesung-sn heesung-sn deleted the transfer-shedder-underloaded-cond branch April 2, 2024 17:43
@heesung-sn heesung-sn restored the transfer-shedder-underloaded-cond branch April 2, 2024 22:59
@heesung-sn heesung-sn reopened this Apr 2, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check
@heesung-sn heesung-sn force-pushed the transfer-shedder-underloaded-cond branch from f4ab491 to 1105476 Compare April 2, 2024 23:01
@heesung-sn heesung-sn merged commit d7d5452 into apache:master Apr 3, 2024
50 checks passed
@heesung-sn heesung-sn deleted the transfer-shedder-underloaded-cond branch April 3, 2024 15:10
heesung-sn added a commit to heesung-sn/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-sn added a commit to heesung-sn/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-sn added a commit to heesung-sn/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-sn added a commit to heesung-sn/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-sn added a commit that referenced this pull request Apr 4, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check (#22321) (#22416)
heesung-sn added a commit that referenced this pull request Apr 4, 2024
…ider max loaded broker's msgThroughputEMA and update IsExtensibleLoadBalancerImpl check (#22321) (#22417)
heesung-sn 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)
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