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

[improve][broker] Change log level to reduce duplicated logs #22147

Merged
merged 4 commits into from Mar 10, 2024

Conversation

mattisonchao
Copy link
Member

@mattisonchao mattisonchao commented Feb 28, 2024

Motivation

Some vendors' environments do not support reading virtual NIC type. The broker will constantly print the useless warning log, which will increase the storage cost.

The demo logs is as follows:

[pulsar-load-manager-1-1] WARN  org.apache.pulsar.broker.loadbalance.LinuxInfoUtils - [LinuxInfo] Failed to read /sys/class/net/nc1 NIC type, the detail is: /sys/class/net/nc1/type: Not a directory
[pulsar-load-manager-1-1] WARN  org.apache.pulsar.broker.loadbalance.LinuxInfoUtils - [LinuxInfo] Failed to read /sys/class/net/nc1 NIC type, the detail is: /sys/class/net/nc1/type: Not a directory
[pulsar-load-manager-1-1] WARN  org.apache.pulsar.broker.loadbalance.LinuxInfoUtils - [LinuxInfo] Failed to read /sys/class/net/nc1 NIC type, the detail is: /sys/class/net/nc1/type: Not a directory

Modifications

  • Because this is the judgement of boolean. It's okay to move the log to be debug level.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

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

@mattisonchao mattisonchao self-assigned this Feb 28, 2024
@mattisonchao mattisonchao added this to the 3.3.0 milestone Feb 28, 2024
@mattisonchao mattisonchao reopened this Feb 28, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 28, 2024
@asafm
Copy link
Contributor

asafm commented Feb 28, 2024

@mattisonchao Can you explain why this log is printed so many times?

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

In this case, the file doesn't even seem to exist. Could we avoid the exception in the first place by checking with java.nio.file.Files#isReadable if the file exists and is readable? Does Files.isReadable check work for these special /sys/class/net files?

@mattisonchao
Copy link
Member Author

mattisonchao commented Feb 29, 2024

@mattisonchao Can you explain why this log is printed so many times?

@asafm Because the broker needs to report the load data constantly and then this judgement method will invoke many times. :)

return stream.filter(LinuxInfoUtils::isPhysicalNic)

@mattisonchao
Copy link
Member Author

mattisonchao commented Feb 29, 2024

In this case, the file doesn't even seem to exist. Could we avoid the exception in the first place by checking with java.nio.file.Files#isReadable if the file exists and is readable? Does Files.isReadable check work for these special /sys/class/net files?

This method is only used to judge whether the current NIC is physical. IMO, the exception status also means not a valid physical NIC. I think giving it a debug-level log for troubleshooting is enough.

And yes, we can give an extra operation to check if the current file type exists. but for the other exceptions, we still need to not print logs in the warn or error level to avoid annoying logs.

cc @lhotari

@lhotari
Copy link
Member

lhotari commented Feb 29, 2024

In this case, the file doesn't even seem to exist. Could we avoid the exception in the first place by checking with java.nio.file.Files#isReadable if the file exists and is readable? Does Files.isReadable check work for these special /sys/class/net files?

This method is only used to judge whether the current NIC is physical. IMO, the exception status also means not a valid physical NIC. I think giving it a debug-level log for troubleshooting is enough.

And yes, we can give an extra operation to check if the current file type exists. but for the other exceptions, we still need to not print logs in the warn or error level to avoid annoying logs.

cc @lhotari

I think it's better to avoid exceptions and add a file existence check since we are touching this part of the code.

@mattisonchao
Copy link
Member Author

Applied the comment. cc @lhotari

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 22.22222% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 38.30%. Comparing base (bbc6224) to head (40b5957).
Report is 27 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #22147       +/-   ##
=============================================
- Coverage     73.57%   38.30%   -35.27%     
+ Complexity    32624    12168    -20456     
=============================================
  Files          1877     1737      -140     
  Lines        139502   132494     -7008     
  Branches      15299    14520      -779     
=============================================
- Hits         102638    50757    -51881     
- Misses        28908    74973    +46065     
+ Partials       7956     6764     -1192     
Flag Coverage Δ
inttests 26.76% <22.22%> (+2.18%) ⬆️
systests 24.26% <22.22%> (-0.07%) ⬇️
unittests 31.83% <22.22%> (-41.01%) ⬇️

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

Files Coverage Δ
...ache/pulsar/broker/loadbalance/LinuxInfoUtils.java 36.64% <22.22%> (-19.71%) ⬇️

... and 1441 files with indirect coverage changes

@mattisonchao mattisonchao merged commit c36c18d into apache:master Mar 10, 2024
50 checks passed
@mattisonchao mattisonchao deleted the fix/duplicated_log branch March 10, 2024 14:07
mattisonchao added a commit that referenced this pull request Mar 13, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 15, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 19, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 23, 2024
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.

None yet

6 participants