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

Expose randomWalk parameters in config #77

Merged
merged 3 commits into from
Feb 25, 2019

Conversation

thomaseizinger
Copy link
Contributor

Implementation of what is suggested in #76 since I need it for a PoC. Happy to revise if it should be done differently :)

@thomaseizinger
Copy link
Contributor Author

I just realized that such a change also needs to be incorporated here:

https://github.com/libp2p/js-libp2p/blob/c4cab007af5e7ec170615941230c18c893eece8a/src/config.js#L34-L40

I'll pause here then and wait for feedback to whether this feature is actually desired.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

let's change the line above for what you proposed in the issue

@@ -164,4 +164,8 @@ class RandomWalk {
}
}

RandomWalk.prototype.defaultQueriesPerPeriod = 1
Copy link
Member

Choose a reason for hiding this comment

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

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 || {}
Copy link
Member

Choose a reason for hiding this comment

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

it would be cleaner to have the defaults here merged with what is received.

@@ -29,9 +29,9 @@ class RandomWalk {
* @returns {void}
*/
start (queries, period, timeout) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

@D4nte
Copy link

D4nte commented Feb 4, 2019

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?

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Feb 5, 2019

libp2p/js-libp2p#317 just got merged 🎉

Regarding the open question from @D4nte :

@vasco-santos How are defaults handled across libp2p modules?
I have the feeling it would be better if a module like js-libp2p-kad-dht is unopinionated about defaults and requires all the config options to be set.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for your contribution.

Can you fix this and fix the commitlint issue?

src/index.js Outdated Show resolved Hide resolved
@vasco-santos
Copy link
Member

@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
Copy link
Contributor Author

thomaseizinger commented Feb 21, 2019

@vasco-santos sorry for the delay!

The code now uses defaultsDeep! Please have another look if you can spot anything else :)

Edit: Jenkins errors with "no space left on device" 😅

thomaseizinger and others added 2 commits February 21, 2019 11:15
- `enabled` for discovery is now inside `randomWalk`
- move default values to constants.js
@ghost ghost assigned vasco-santos Feb 21, 2019
@ghost ghost added the status/in-progress In progress label Feb 21, 2019
@vasco-santos
Copy link
Member

@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!

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

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

@thomaseizinger
Copy link
Contributor Author

@vasco-santos I wasn't sure how you exactly meant the docs to be fixed so I took a guess at it :D

@vasco-santos
Copy link
Member

It was a good guess then 🎉

Thanks for the PR @thomaseizinger

@vasco-santos vasco-santos merged commit dc5a67f into libp2p:master Feb 25, 2019
@ghost ghost removed the status/in-progress In progress label Feb 25, 2019
@thomaseizinger thomaseizinger deleted the 76-random-walk-parameters branch March 4, 2019 20:57
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