Skip to content

[fix][broker] Fix incorrect Nic usage collected by pulsar - #18882

Merged
Technoboy- merged 3 commits into
apache:masterfrom
thetumbled:fixbug_filterDownNic
Dec 14, 2022
Merged

[fix][broker] Fix incorrect Nic usage collected by pulsar#18882
Technoboy- merged 3 commits into
apache:masterfrom
thetumbled:fixbug_filterDownNic

Conversation

@thetumbled

@thetumbled thetumbled commented Dec 12, 2022

Copy link
Copy Markdown
Member

Fixes #18881 #18881

Motivation

When retrieve all paths of physical NICs, it did not filter those NICs whose state are down.
org.apache.pulsar.broker.loadbalance.LinuxInfoUtils#getPhysicalNICs
image

As a consequence, we will get a wrong limit value of NIC if loadBalancerOverrideBrokerNicSpeedGbps is enabled.
image

for example, we have 4 physical nics, but only eth0 and eth1 is enable, and both of their speed is 10Gbps.

2: eth0:  state UP 
3: eth1: state UP 
4: eth2: state DOWN 
5: eth3:state DOWN 

So, the correct limit value of NIC is 20Gbps, that is 20000000 Kbps. But the limit value collected by pulsar is 40Gbps, that is 40000000 Kbps. this is caused by the bug described above.

Modifications

filter those NICs who are unusable.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as (please describe tests).

Documentation

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

Matching PR in forked repository

PR in forked repository: thetumbled#7

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Dec 12, 2022
@mattisonchao

mattisonchao commented Dec 12, 2022

Copy link
Copy Markdown
Member

Sorry, I missed this PR. could you follow this change? #18883 Because from the kernel document, not only up state can work.

@thetumbled

Copy link
Copy Markdown
Member Author

Sorry, I missed this PR. could you follow this change? #18883 Because from the kernel document, not only up state can work.

ok.

@Technoboy- Technoboy- changed the title [fix] [broker] incorrect Nic usage collected by pulsar [fix][broker] Fix incorrect Nic usage collected by pulsar Dec 12, 2022
@Technoboy- Technoboy- added this to the 2.12.0 milestone Dec 12, 2022
@Technoboy- Technoboy- closed this Dec 12, 2022
@Technoboy- Technoboy- reopened this Dec 12, 2022
@codecov-commenter

codecov-commenter commented Dec 12, 2022

Copy link
Copy Markdown

Codecov Report

Merging #18882 (1dd8469) into master (3180a4a) will increase coverage by 0.59%.
The diff coverage is 56.52%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18882      +/-   ##
============================================
+ Coverage     46.17%   46.76%   +0.59%     
- Complexity    10359    10500     +141     
============================================
  Files           703      703              
  Lines         68845    68870      +25     
  Branches       7382     7384       +2     
============================================
+ Hits          31788    32208     +420     
+ Misses        33448    33051     -397     
- Partials       3609     3611       +2     
Flag Coverage Δ
unittests 46.76% <56.52%> (+0.59%) ⬆️

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

Impacted Files Coverage Δ
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 15.09% <0.00%> (-0.04%) ⬇️
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 22.78% <0.00%> (-0.09%) ⬇️
...ache/pulsar/broker/loadbalance/LinuxInfoUtils.java 34.65% <75.00%> (+7.90%) ⬆️
...ker/loadbalance/impl/LinuxBrokerHostUsageImpl.java 79.68% <100.00%> (ø)
...g/apache/bookkeeper/mledger/util/StatsBuckets.java 43.75% <0.00%> (-16.67%) ⬇️
...ookkeeper/mledger/impl/ManagedLedgerMBeanImpl.java 53.17% <0.00%> (-9.53%) ⬇️
...sar/broker/service/schema/SchemaRegistryStats.java 71.25% <0.00%> (-3.75%) ⬇️
...oker/service/schema/SchemaRegistryServiceImpl.java 61.84% <0.00%> (-3.47%) ⬇️
...ker/resourcegroup/ResourceGroupConfigListener.java 64.38% <0.00%> (-2.74%) ⬇️
...ava/org/apache/pulsar/broker/service/Consumer.java 66.19% <0.00%> (-1.96%) ⬇️
... and 48 more

@MarvinCai MarvinCai 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

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.

[Bug] incorrect Nic usage collected by pulsar

5 participants