handling IllegalArgumentException caused by Discovery Disabled Nodes in Endpoint.fromEnode #7937
Conversation
Signed-off-by: vaidikcode <[email protected]>
| Optional<Integer> discoveryPort = enode.getDiscoveryPort(); | ||
|
|
||
| if (discoveryPort.isEmpty()) { | ||
| log.warn("Attempted to create a discovery endpoint for a node with discovery disabled: {}", enode); |
There was a problem hiding this comment.
if the discovery port is empty, the default port will be used, right? Can we add a test for the expected outcome?
And in that case, should this log be moved to debug? "Using default port for enode"
Signed-off-by: vaidikcode <[email protected]>
ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/Endpoint.java
Outdated
Show resolved
Hide resolved
...um/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java
Show resolved
Hide resolved
Signed-off-by: vaidikcode <[email protected]>
Signed-off-by: vaidikcode <[email protected]>
Signed-off-by: vaidikcode <[email protected]>
Signed-off-by: vaidikcode <[email protected]>
ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/Endpoint.java
Outdated
Show resolved
Hide resolved
| when(enodeWithNoDiscovery.getIp()).thenReturn(InetAddress.getByName("127.0.0.1")); | ||
| } | ||
| catch (UnknownHostException e) { | ||
| log.debug("Failed to resolve the Host Address "); |
There was a problem hiding this comment.
prob wouldn't catch this exception in the test, you can mark the test method throws that ex. (what would happen if this exception occurred? would the test pass or fail? should it pass or fail?)
There was a problem hiding this comment.
I don't believe this will significantly impact the test's primary function. Should I proceed with Throws it?
There was a problem hiding this comment.
yes I would keep this logic out of the test altogether
|
|
||
| public class PeerDiscoveryAgentTest { | ||
|
|
||
| private static final int BROADCAST_TCP_PORT = 30303; | ||
| private static final Supplier<SignatureAlgorithm> SIGNATURE_ALGORITHM = | ||
| Suppliers.memoize(SignatureAlgorithmFactory::getInstance); | ||
| private static final Logger log = LoggerFactory.getLogger(PeerDiscoveryAgentTest.class); |
There was a problem hiding this comment.
generally don't need loggers in tests
Signed-off-by: vaidikcode <[email protected]>
…iscovery/Endpoint.java Co-authored-by: Sally MacFarlane <[email protected]> Signed-off-by: Vaidik <[email protected]>
Signed-off-by: vaidikcode [email protected]
PR description
This PR modifies the Endpoint.fromEnode method to handle cases where discovery is disabled for a given EnodeURL more gracefully. Previously, the method threw an IllegalArgumentException when getDiscoveryPort returned an empty Optional, leading to potential disruptions in the system.
Fixed Issue(s)
#7887
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests