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

feat: retry crawl if peer is down #357

Closed
wants to merge 1 commit into from
Closed

Conversation

michielbdejong
Copy link
Contributor

No description provided.

Copy link
Contributor

@sentientwaffle sentientwaffle left a comment

Choose a reason for hiding this comment

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

_crawlClient() will return immediately even if it is waiting to retry, which is misleading. Also, if the retry fails it will do so silently.

@michielbdejong
Copy link
Contributor Author

You want the retries to happen in the background (the connector should already start broadcasting to the peers that were crawled so far), but you're right, I'll see if I can find a cleaner way to achieve that. Maybe we should rename the function to 'startCrawlClient' or something, to make it clearer

@codecov-io
Copy link

codecov-io commented Jul 4, 2017

Codecov Report

Merging #357 into master will decrease coverage by 3.86%.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
- Coverage   82.42%   78.56%   -3.87%     
==========================================
  Files          34       34              
  Lines        1195     1199       +4     
  Branches      189      189              
==========================================
- Hits          985      942      -43     
- Misses        210      257      +47
Impacted Files Coverage Δ
src/lib/route-broadcaster.js 49.03% <11.11%> (-26.47%) ⬇️
src/lib/routing-tables.js 77.77% <0%> (-6.67%) ⬇️
src/lib/ledgers.js 75.3% <0%> (-2.47%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91c2a6a...8ddd32f. Read the comment docs.

// follow-up attempts happen in the background until successful
// but this generator function already returns
setTimeout(() => {
this._crawlClient(client)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this function no longer exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah :) sorry. fixed.

// follow-up attempts happen in the background until successful
// but this generator function already returns
setTimeout(() => {
this._startCrawlClient(client)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this need to be in a co(), since _startCrawlClient is a generator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I should add a unit test to this PR :)

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Aug 17, 2017

@sentientwaffle I vaguely remember you solved this issue (and the underlying interledger-deprecated/ilp-kit#435) in a different PR? If not I'll take another look at this once I'm back from holidays.

@sentientwaffle
Copy link
Contributor

I think you're right, #386 should fix this issue. When the plugin connects _crawlLedgerPlugin is triggered.

@michielbdejong
Copy link
Contributor Author

ok, will close this then. we should test this in the next version of ilp-kit to check if it's really fixed now.

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