Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

Fix p2p PeerInfo handling #1428

Merged
merged 7 commits into from
May 10, 2019

Conversation

mbaxter
Copy link
Contributor

@mbaxter mbaxter commented May 9, 2019

PR description

This PR reworks handling of potentially null / empty values in DefaultP2PNetwork. PeerInfo is wrapped in an Optional to make it more obvious that this value may be empty. Some additional checks have been added to shortcut operations that should only execute when the network is fully up and running:

  • Peers are not permitted to connect until the network is fully up
  • connect() will return an exceptional result if invoked before the network is up

@mbaxter mbaxter marked this pull request as ready for review May 9, 2019 16:45
@mbaxter mbaxter requested a review from rain-on May 9, 2019 16:45
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM. When the network has started, should we trigger a check of any maintained peers to ensure we connect to them rather than waiting for the periodic check to kick in? Admittedly the initial check is only a 2 second delay so it isn't much of a difference but seems like that initial delay could be 0 (line 556 of DefaultP2PNetwork).

@mbaxter
Copy link
Contributor Author

mbaxter commented May 10, 2019

This is not the BEST rationale, but I set the initial timeout to 2 rather 0 to make tests a little simpler. There are some tests where an immediately scheduled check interferes because the scheduled check may or may not run during the test.

@mbaxter mbaxter merged commit 141b9c5 into PegaSysEng:master May 10, 2019
notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants