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

Async Refactor: random-walk #134

Merged
merged 8 commits into from
Jun 11, 2019
Merged

Async Refactor: random-walk #134

merged 8 commits into from
Jun 11, 2019

Conversation

kumavis
Copy link
Contributor

@kumavis kumavis commented Jun 3, 2019

switched over to a _running state var and and while(true) async loop instead of managing timeoutIds

skipped a test that was expecting queries to be run in series and that errors would stop the other queries. Should an error in _walk trigger randomWalk to stop? currently we log and ignore

updates #82

@kumavis kumavis requested review from vasco-santos and jacobheun and removed request for vasco-santos June 3, 2019 16:18
src/random-walk.js Outdated Show resolved Hide resolved
src/random-walk.js Outdated Show resolved Hide resolved
@dirkmc
Copy link
Contributor

dirkmc commented Jun 3, 2019

I don't think an error should cause random walk to stop - we don't really care about past errors for the purposes of random walk, we just want to walk the DHT to find new peers

@kumavis
Copy link
Contributor Author

kumavis commented Jun 3, 2019

@dirkmc i skip()ed a test that expects an error to stop the query, do you think i should delete the test?

@kumavis kumavis mentioned this pull request Jun 3, 2019
@dirkmc
Copy link
Contributor

dirkmc commented Jun 3, 2019

I would say we should have a test for the opposite case - that the random walk does continue even if there's an error, but I would like to hear from @vasco-santos and @jacobheun if they know of a reason why it was written this way originally.

@jacobheun
Copy link
Contributor

I would say we should have a test for the opposite case - that the random walk does continue even if there's an error.

I agree, there's no reason to stop the random walk on an error. The only reason the walk should stop is if the query is done or it timed out.

@vasco-santos
Copy link
Member

Agree that it should keep going!

@kumavis kumavis force-pushed the feat/async-random-walk branch from fdea996 to e5946d2 Compare June 9, 2019 15:38
@kumavis
Copy link
Contributor Author

kumavis commented Jun 9, 2019

@vasco-santos @jacobheun ready to go

@kumavis
Copy link
Contributor Author

kumavis commented Jun 10, 2019

fixed the conflict

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 another PR @kumavis

Just left a minor request

package.json Show resolved Hide resolved
src/random-walk.js Show resolved Hide resolved
src/random-walk.js Outdated Show resolved Hide resolved
Co-Authored-By: Vasco Santos <[email protected]>
@kumavis
Copy link
Contributor Author

kumavis commented Jun 11, 2019

@vasco-santos I'll handle the multihashing-async update in a separate PR, as it will affect more files than randomWalk

@vasco-santos vasco-santos merged commit a8f1df5 into master Jun 11, 2019
@vasco-santos vasco-santos deleted the feat/async-random-walk branch June 11, 2019 10:44
} catch (err) {
this._kadDHT._log.error('random-walk:error', err)
}
// Each subsequent walk should run on a `this._options.interval` interval
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be checking this._timeoutId has not been cleared after the walk has completed?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point! We can check and not do the timeout if needed.

src/random-walk.js Show resolved Hide resolved
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.

4 participants