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

Remove signal bootstrapping #224

Merged
merged 1 commit into from
Jan 22, 2019
Merged

Remove signal bootstrapping #224

merged 1 commit into from
Jan 22, 2019

Conversation

anacrolix
Copy link
Contributor

Remove IpfsDHT.BootstrapOnSignal. This is behaviour that can be achieved without intruding on IpfsDHT. It's not used elsewhere. The tests are retained and adapted to work without the method.

Remove IpfsDHT.BootstrapOnSignal.
@ghost ghost assigned anacrolix Jan 22, 2019
@ghost ghost added the status/in-progress In progress label Jan 22, 2019
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I agree it doesn't really belong but... it's used in ipfs-cluster. cc @hsanjuan

@anacrolix
Copy link
Contributor Author

There's a failing test but I believe it's unrelated, and I'm going to move on (and maybe fix that test in another PR).

@anacrolix anacrolix merged commit 838d43d into master Jan 22, 2019
@ghost ghost removed the status/in-progress In progress label Jan 22, 2019
@anacrolix anacrolix deleted the remove-signal-bootstrap branch January 22, 2019 22:47
@hsanjuan
Copy link
Contributor

Hey, we use this. We need this. Why was this merged without even giving me the chance to explain why it's useful?

@hsanjuan
Copy link
Contributor

At least explain what the workaround is.

@hsanjuan hsanjuan restored the remove-signal-bootstrap branch January 23, 2019 12:26
@vyzo
Copy link
Contributor

vyzo commented Jan 23, 2019

people please refrain merging those refactorings before the stakeholders can review!

@hsanjuan
Copy link
Contributor

FYI, this is used because it is impossible to make the DHT do a bootstrap round at a certain moment. For example, if a new peer is added is added to a cluster, that peer needs to bootstrap ASAP, not wait for the next automatic bootstrap round. I don't think there is an equivalent way to manually trigger a bootstrap round.

@anacrolix
Copy link
Contributor Author

@hsanjuan Thanks for the clarification. I don't find any other uses of BootstrapOnSignal other than the one in ipfs-cluster. It looks like the actual desired API is a synchronous Bootstrap method. I propose instead to add a method that does a single bootstrap synchronously.

@Stebalien
Copy link
Member

@anacrolix In the future, please don't knowingly break someone else's code without their input (or very careful consideration).

@anacrolix
Copy link
Contributor Author

This has been rolled into #225.

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.

5 participants