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

p2p/pex: consult seeds in crawlPeersRoutine #3647

Merged
merged 2 commits into from
May 21, 2019
Merged

p2p/pex: consult seeds in crawlPeersRoutine #3647

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f3c4bc
p2p/pex: consult seeds in crawlPeersRoutine
May 8, 2019
4a08499
p2p/pex: Adjust error message for no peers
defunctzombie May 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions p2p/pex/pex_reactor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func (r *PEXReactor) OnStart() error {
if err != nil {
return err
} else if numOnline == 0 && r.book.Empty() {
return errors.New("Address book is empty, and could not connect to any seed nodes")
return errors.New("Address book is empty and no seed nodes")
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message could be more clear and better synchronized with the result of checkSeeds.

For instance, checkSeeds could return -1 and a nil error to indicate there are no seeds, which isn't captured here.

checkSeeds should probably do a better job of returning errors, and for instance return an error indicating there are no seeds rather than using the special case of -1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you prefer the error message to say?

Do you want me to make changes to return values for checkSeeds - largely I tried to minimize the diff but if you have a strategy/direction you want this to go I am happy to make the changes. I am not familiar with the proper way to return and handle different "types" of errors in Go or what you prefer for tendermint so just let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries I realized we actually don't want to error on empty seeds list since that would cause the node not to start, which isn't what we want. So everything is fine. If you could just accept the one suggestion we can merge. Thanks for the fix!

defunctzombie marked this conversation as resolved.
Show resolved Hide resolved
}

r.seedAddrs = seedAddrs
Expand Down Expand Up @@ -573,7 +573,7 @@ func (r *PEXReactor) checkSeeds() (numOnline int, netAddrs []*p2p.NetAddress, er
return 0, nil, errors.Wrap(e, "seed node configuration has error")
}
}
return
return numOnline, netAddrs, nil
}

// randomly dial seeds until we connect to one or exhaust them
Expand Down Expand Up @@ -608,8 +608,13 @@ func (r *PEXReactor) AttemptsToDial(addr *p2p.NetAddress) int {
// Seed/Crawler Mode causes this node to quickly disconnect
// from peers, except other seed nodes.
func (r *PEXReactor) crawlPeersRoutine() {
// Do an initial crawl
r.crawlPeers(r.book.GetSelection())
// If we have any seed nodes, consult them first
if len(r.seedAddrs) > 0 {
r.dialSeeds()
} else {
// Do an initial crawl
r.crawlPeers(r.book.GetSelection())
}

// Fire periodically
ticker := time.NewTicker(crawlPeerPeriod)
Expand Down