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.

Sign up for GitHub

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

Jump to bottom

Filter Netty native lib errors likewise the pure Java implementation #3807

Merged
merged 5 commits into from
May 11, 2022
Merged

Filter Netty native lib errors likewise the pure Java implementation #3807

merged 5 commits into from
May 11, 2022

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented May 9, 2022 •
edited
Loading

Signed-off-by: Fabio Di Fabio fabio.difabio@consensys.net

PR description

Since version 22.4.0 Netty prefers the native lib implementation, instead of the pure Java implementation, and this causes warn log messages in case IPv6 is not enable or configured. These error messages are intercepted and logged at debug when thrown by the pure Java implementation, but the native lib implementation triggers a different exception, so the scope of this PR is to extend the error filtering logic to intercept and log at debug the equivalent errors thrown by the native lib implementation

Fixed Issue(s)

fixes #3798

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

@fab-10 fab-10 force-pushed the 3798-network-is-unreachable branch from 306afd2 to 29e1f7b Compare May 9, 2022 18:07
@fab-10 fab-10 changed the title Extend the PeerDiscovery error filtering to handle native errors Filter Netty native lib errors likewise the pure Java implementation May 9, 2022
@fab-10
Filter Netty native lib errors likewise the pure Java implementation
87eb377 
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 force-pushed the 3798-network-is-unreachable branch from 29e1f7b to 87eb377 Compare May 9, 2022 18:18
@fab-10 fab-10 marked this pull request as ready for review May 9, 2022 18:18
diega
diega approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@diega diega left a comment

Choose a reason for hiding this comment

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

LGTM.
I just left some non blocking comments

gradle/versions.gradle Outdated Show resolved Hide resolved
ethereum/p2p/build.gradle Outdated Show resolved Hide resolved
ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgent.java Outdated Show resolved Hide resolved
macfarla
macfarla reviewed May 10, 2022
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

sonar is complaining about code coverage - can we add a test?

@fab-10
Implement code review comments
2332c96 
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10
Copy link
Contributor Author

fab-10 commented May 10, 2022

sonar is complaining about code coverage - can we add a test?
@macfarla unfortunately there are no unit tests at all for that specific class VertxPeerDiscoveryAgent 😞, and I think it makes sense to create tests for the entire class, not only for my minor modification, do you think we can release this, and delegate the unit tests creation to another ticket?

@macfarla
Copy link
Contributor

sonar is complaining about code coverage - can we add a test?
@macfarla unfortunately there are no unit tests at all for that specific class VertxPeerDiscoveryAgent 😞, and I think it makes sense to create tests for the entire class, not only for my minor modification, do you think we can release this, and delegate the unit tests creation to another ticket?

I think there is already a ticket - #3420 - so yes I suppose so!

macfarla
macfarla approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

LGTM. Q re traceLambda

...m/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/VertxPeerDiscoveryAgent.java Outdated
LOG.trace(
"Sending to peer {} failed, packet: {}, stacktrace: {}",
peer,
wrapBuffer(packet.encode()),
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a spot for traceLambda?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the hope is that this error is not frequent, but yes it make sense to use lambda version for this and the debug logs

@fab-10
use lambda for trace and debug logs
0ea0aa3 
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 added the mainnet label May 10, 2022
macfarla
macfarla approved these changes May 11, 2022
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented May 11, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@fab-10 fab-10 merged commit 3869459 into hyperledger:main May 11, 2022
@fab-10 fab-10 deleted the 3798-network-is-unreachable branch May 11, 2022 06:54
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@fab-10 @eum602
Filter Netty native lib errors likewise the pure Java implementation (h…
5a190a7 
…yperledger#3807)


Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diega diega diega approved these changes

@macfarla macfarla macfarla approved these changes

Assignees
No one assigned
Labels
mainnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

io.netty.channel.unix.Errors$NativeIoException: sendToAddress(..) failed: Network is unreachable
3 participants
@fab-10 @macfarla @diega