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

Support unannounce, move to maxAge instead of timeslotting #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mafintosh
Copy link
Contributor

@mafintosh mafintosh commented Jan 9, 2018

Implements support for distributed unannounce using webtorrent/bittorrent-dht#184 which means that services when shutting down gracefully can tell the DHT to unannounce itself (NOTE: if we want to land this, we shouldn't merge it until the DHT pr has landed most likely).

Also moves the timeslotting behaivor to use maxAge instead, since this is the intended use of that.
If you want I can split up the PR in two parts, one for timeslotting and one for unannounce, but the two go nicely hand in hand.

If we land unannounce, I suggest we increase the default timeslot to something more like 2-5 min, since services most of the time will be able to unannounce themself, so there will be less dead nodes in the dht. This should drastically reduce the announce traffic on the dht

The link client PR that allows a link to unannounce is here, bitfinexcom/grenache-nodejs-link#10

Let me know what you think :)

@mafintosh
Copy link
Contributor Author

Forgot to mention, I was gonna add tests, if we wanna land this of course

@prdn
Copy link
Collaborator

prdn commented Jan 9, 2018

I like the idea a lot.
At first I was thinking: if we increase the timeslot then we are in the situation where if something fails is harder to find it out.
But in a real world scenario we have redundant workers for each service. So we'll see the clients periodically (randomly) trying to hit dead services for 2/5 mins but we should be likely in the situation where most of the other workers (for the same service) are alive, so the error rate should be contained.
Thanks @mafintosh

@mafintosh
Copy link
Contributor Author

@prdn yea, and with unannounce we can unannounce a bad service manually, assuming you are on the box with the same hostname, so we might want to add an cli that could that that.

most of the time services would unannounce gracefully as well, so the 2-5 min failure scenario is only a problem when something has a catastrophic failure. will still happen but less likely

@prdn
Copy link
Collaborator

prdn commented Jan 10, 2018

@mafintosh @robertkowalski wondering if we can remove the timeslot all together.
If we make https://github.com/webtorrent/bittorrent-dht/blob/master/client.js#L781 configurable also with a time span, then we can avoid the timeslot on grenache-grape
Right now the timeslot is a parameter that needs to be shared exactly by all the grapes, while without it network configuration would be easier.

@robertkowalski
Copy link
Contributor

robertkowalski commented Jan 10, 2018

@prdn good point! talked to @mafintosh, basically that was the intention, but the maxAge option in bittorrent dht was not used. added it in webtorrent/bittorrent-dht#186

@prdn
Copy link
Collaborator

prdn commented Jan 10, 2018

@robertkowalski can you open a separate PR for Grape, to allow the configuration of the maxAge parameter for PeerStore (also in the command line version)?

@robertkowalski
Copy link
Contributor

sure! had to add tests, will do when the patch landed in dht master

@@ -45,7 +45,8 @@ class Grape extends Events {
host: this.conf.host || false,
bootstrap: this.conf.dht_bootstrap,
timeBucketOutdated: this.conf.dht_nodeLiveness,
verify: crypto.verify
verify: crypto.verify,
maxAge: this.conf.timeslot
Copy link
Collaborator

@prdn prdn Jan 11, 2018

Choose a reason for hiding this comment

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

@mafintosh can we call this dht_timeslot or dht_peer_maxAge ?
edit: I'm referring to this.conf.timeslot that should be renamed into dht_peer_maxAge

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

3 participants