Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

feat: start random walk and allow configuration for disabling #42

Merged
merged 4 commits into from
Oct 1, 2018

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Sep 24, 2018

In the context of the integration of the DHT to js-ipfs js-ipfs#856.

I changed the implementation of randomWalk. It was using a setInterval, which was causing this function to run the _walk function after five minutes. On the contrary, go-libp2p-kad-dht runs immediately after the node starts and then, each 5 minutes, as we can see in go-libp2p-kad-dht/dht_bootstrap.go#L81-L85

Worklist:

  • Start random-walk once the DHT is started
  • Accept configuration for disabling the default behavior

@ghost ghost assigned vasco-santos Sep 24, 2018
@ghost ghost added the status/in-progress In progress label Sep 24, 2018
src/index.js Outdated
if (err) {
return callback(err)
}
this.randomWalk.start()
Copy link
Member

Choose a reason for hiding this comment

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

This should be started just like any other Peer Discovery Service, that means, start it on js-libp2p instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will change the PR and create a new one for js-libp2p! Thanks

@vasco-santos vasco-santos changed the title fix: start random walk on start fix: start random walk once it is started Sep 24, 2018
@vasco-santos vasco-santos force-pushed the fix/start-random-walk branch 2 times, most recently from 3f1abd9 to 45dfb33 Compare September 24, 2018 09:39
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

@vasco-santos the code looks good but it looks like there is an issue with libp2p-mplex that we should fix. Since this is going to be running periodically and by default we need to avoid any uncaught errors.

Looking at the CI error we need to be checking drained here https://github.com/libp2p/js-libp2p-mplex/blob/v0.8.0/src/internals/index.js#L167-L169. If L160 sets drained to false, pushing again will error, which is what CI is not happy about.

@vasco-santos
Copy link
Member Author

I will look into that problem @jacobheun . Thanks for finding it out

@vasco-santos
Copy link
Member Author

@jacobheun it is green now!

@diasdavid can we have your feedback to move this forward?
Explanation in this discussion

@vasco-santos vasco-santos changed the title fix: start random walk once it is started feat: start random walk (and allow configuration for disabling) Oct 1, 2018
@vasco-santos vasco-santos changed the title feat: start random walk (and allow configuration for disabling) feat: start random walk and allow configuration for disabling Oct 1, 2018
Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Go for it.

@vasco-santos vasco-santos merged commit abe9407 into master Oct 1, 2018
@ghost ghost removed the status/in-progress In progress label Oct 1, 2018
@vasco-santos vasco-santos deleted the fix/start-random-walk branch October 1, 2018 13:03
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.

3 participants