Expose randomWalk parameters in config #77
Conversation
thomaseizinger
force-pushed
the
76-random-walk-parameters
branch
from
eeb97cd
to
d9dc50b
Compare
|
I just realized that such a change also needs to be incorporated here: I'll pause here then and wait for feedback to whether this feature is actually desired. |
There was a problem hiding this comment.
Thanks for creating this PR @thomaseizinger ! 👍
I just asked for some changes to merge this
src/index.js
Outdated
| @@ -116,6 +117,9 @@ class KadDHT { | |||
| * Random walk state, default true | |||
| */ | |||
| this.randomWalkEnabled = !options.hasOwnProperty('enabledDiscovery') ? true : Boolean(options.enabledDiscovery) | |||
There was a problem hiding this comment.
let's change the line above for what you proposed in the issue
src/random-walk.js
Outdated
| @@ -164,4 +164,8 @@ class RandomWalk { | |||
| } | |||
| } | |||
|
|
|||
| RandomWalk.prototype.defaultQueriesPerPeriod = 1 | |||
There was a problem hiding this comment.
Please, add these to to constants file instead
src/index.js
Outdated
| @@ -44,6 +44,7 @@ class KadDHT { | |||
| options = options || {} | |||
| options.validators = options.validators || {} | |||
| options.selectors = options.selectors || {} | |||
| options.randomWalk = options.randomWalk || {} | |||
There was a problem hiding this comment.
it would be cleaner to have the defaults here merged with what is received.
src/random-walk.js
Outdated
| @@ -29,9 +29,9 @@ class RandomWalk { | |||
| * @returns {void} | |||
| */ | |||
| start (queries, period, timeout) { | |||
There was a problem hiding this comment.
Please remove the lines below and change the above for using the default parameters, as follows:
start (queries = ..., )I know the function was not implemented by you, but feels like a good oportunity to change this.
|
Changes done, however before merging... In js-libp2p default values are set when declaring the configuration libp2p/js-libp2p#317 Should we make this library unopinionated on how often the walk should run and let the caller, js-libp2p, set their own defaults? |
|
libp2p/js-libp2p#317 just got merged 🎉 Regarding the open question from @D4nte : @vasco-santos How are defaults handled across libp2p modules? |
|
@thomaseizinger I want to have the defaults here too, as this implementation may be used by other projects, and it is better to fallback to a value than to require all the parameters. |
thomaseizinger
force-pushed
the
76-random-walk-parameters
branch
from
90c93a7
to
9ebd0bb
Compare
|
@vasco-santos sorry for the delay! The code now uses Edit: Jenkins errors with "no space left on device" 😅 |
- `enabled` for discovery is now inside `randomWalk` - move default values to constants.js
vasco-santos
force-pushed
the
76-random-walk-parameters
branch
from
9ebd0bb
to
7fbbdd3
Compare
ghost
assigned
|
@thomaseizinger thanks for the updates! Basically, we are moving our CI from jenkins to travis now. So, the master branch already is using travis, but this PR came from an older version of it. I just rebased your PR with that to use travis as well! I will have a look to the PR! |
There was a problem hiding this comment.
Thanks for the update @thomaseizinger
We are really close now! This looks good, just a minor detail regarding jsdoc. Can you add it?
| @@ -46,6 +47,7 @@ class KadDHT extends EventEmitter { | |||
| options = options || {} | |||
| options.validators = options.validators || {} | |||
| options.selectors = options.selectors || {} | |||
| options.randomWalk = defaultsDeep(options.randomWalk, c.defaultRandomWalk) | |||
There was a problem hiding this comment.
Add options.randomWalk to the js docs above and describe the options type for it as follows:
/**
* Random walk options
* @typedef {Object} randomWalk
* @property {boolean} enabled - discovery enabled (default: true)
* @property {number} queriesPerPeriod - how many queries to run per period (default: 1)
* @property {number} interval - how often to run the the random-walk process, in milliseconds (default: 300000)
* @property {number} timeout - how long to wait for the the random-walk query to run, in milliseconds (default: 10000)
*/Also remove options.enabledDiscovery from the docs
|
@vasco-santos I wasn't sure how you exactly meant the docs to be fixed so I took a guess at it :D |
|
It was a good guess then 🎉 Thanks for the PR @thomaseizinger |
Implementation of what is suggested in #76 since I need it for a PoC. Happy to revise if it should be done differently :)