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

chore: update kbucket #549

Merged
merged 2 commits into from
Apr 6, 2020
Merged

chore: update kbucket #549

merged 2 commits into from
Apr 6, 2020

Conversation

Stebalien
Copy link
Member

No description provided.

@Stebalien Stebalien requested a review from aschmahmann April 6, 2020 02:56
@@ -222,10 +222,10 @@ func (dht *IpfsDHT) refreshCpls(ctx context.Context) error {
return err
}

if err := doQuery(tcpl.Cpl, randPeer.String(), walkFnc); err != nil {
if err := doQuery(uint(cpl), randPeer.String(), walkFnc); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: #550 (in a diferent PR)

@aschmahmann
Copy link
Contributor

TestFindPeerQueryMinimal is failing now because this kbucket update fixes refreshes to query buckets that have not been queried before. AFAICT what's happening in the test is that the test probabilistically leaves some of the leaf nodes totally disconnected. This leads to the test throwing an error (recently introduced by me because I wanted to make the network more connected now that we rely more heavily on good routing tables).

Some options:

  1. Ignore the orphaned leaves
    a. Skip the DHTs that have empty routing tables, or
    b. Just ignore kb.ErrLookupFailure errors
  2. Make the test better
    a. Don't let random leaves be orphaned
    b. Make the test generate semi real routing tables

My recommendation here is to do 1a, it's easy and keeps the test true to how it was behaving before this PR.

…nd refresh routing tables for orphaned nodes
Comment on lines +1417 to +1420
if len(d.RoutingTable().ListPeers()) > 0 {
if err := <-d.RefreshRoutingTable(); err != nil {
t.Fatal(err)
}
Copy link
Contributor

@aschmahmann aschmahmann Apr 6, 2020

Choose a reason for hiding this comment

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

This test had to be modified because of #549 (comment)

@Stebalien Stebalien merged commit a7fd35f into cypress Apr 6, 2020
@Stebalien Stebalien deleted the chore/update-kbucket branch April 6, 2020 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants