-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
6671c38
to
393cf50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a cursory first pass.
…factored BackoffDiscovery to better differentiate the BackoffStrategy from BackoffFactory.
…into feat/backoff
c839e68
to
e7f9582
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need docstrings here, and also do something about the funky names of some procedures.
Also, we should add utility methods with known good coefficients that reduce the burden of using this API.
@vyzo not sure why Github won't show the outdated/the updated comments in the conversation tab, but you can see them in the files tab. Also, I moved the mock discovery used by this and the previous PR into a new file both can use. |
Can you add some utility procedures for known good coefficients and the such? |
Would you like me to emulate the one from the dialer https://github.com/libp2p/go-libp2p-swarm/blob/99831444e78c8f23c9335c17d8f7c700ba25ca14/swarm_dial.go which is
This is literally just math, what the right backoff times/rates are is a whole separate thing. The dialer says it uses quadratic instead of exponential backoff, but no reasons are given. All over the internet you can find resources on designing/benchmarking backoff strategies (e.g. https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/). For each service we use backoff in (e.g. pubsub, dialer, etc.) we should have a default. Don't see why that should be here though. Side note: @raulk would you have any interest using this in the dialer? There are some issues about changing/upgrading the backoff e.g. libp2p/go-libp2p#1554 |
Because the API is unusable without it? People using the API should have a simple way to get reasonable defaults. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. I'm concerned about the complexity of these features but I can't think of a significantly simpler way to implement them.
@Stebalien Me too. Idk how frequently we're going to need a caching structure wrapped around an event system that needs to catch up on previous events. However, if something similar comes up again we should just turn a lot of these into |
🚀 |
To deal with repeated discovery requests (as seems likely to occur in libp2p/go-libp2p-pubsub#184) we need some sort of caching + backoff scheme.
These are a few configurable schemes to get us moving.
This also relates to libp2p/go-libp2p#707