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

Run deal restarts in go routine #309

Merged
merged 1 commit into from
Jul 2, 2020
Merged

Conversation

hannahhoward
Copy link
Collaborator

Goals

Deal restarts make take a while particularly if there are connection timeouts. There's no reason to
run them synchronously in node initialization, and we definitely should not fail node startup if there are errors.

Implementation

Run restart deals in a go routine, simply log any errors

@hannahhoward hannahhoward force-pushed the fix/restart-deals-error branch from a5c9555 to 90b59ce Compare July 2, 2020 03:45
Deal restarts make take a while particularly if there are connection timeouts. There's no reason to
run them synchronously in node initialization
@hannahhoward hannahhoward force-pushed the fix/restart-deals-error branch from 90b59ce to 21be96a Compare July 2, 2020 03:46
@codecov-commenter
Copy link

Codecov Report

Merging #309 into master will decrease coverage by 0.25%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #309      +/-   ##
==========================================
- Coverage   63.65%   63.41%   -0.24%     
==========================================
  Files          41       41              
  Lines        2569     2574       +5     
==========================================
- Hits         1635     1632       -3     
- Misses        813      820       +7     
- Partials      121      122       +1     
Impacted Files Coverage Δ
storagemarket/impl/client.go 3.20% <0.00%> (-0.05%) ⬇️
storagemarket/impl/provider.go 4.41% <0.00%> (-0.01%) ⬇️
retrievalmarket/impl/blockio/traverser.go 68.89% <0.00%> (-3.33%) ⬇️

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 4fcaee3...21be96a. Read the comment docs.

Copy link
Collaborator

@arajasek arajasek 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 the quick fix!

I'm guessing we probably want some way to automatically attempt the deal restart again at some point? Maybe crack an issue or track that somewhere?

@hannahhoward
Copy link
Collaborator Author

@arajasek I think I'm gonna create a meta issue in markets that's like "deal reliability tracking" cause there are several things we may want to do. Note the current version of markets does not attempt to reopen connections btw cause the newest protocol has no long running libp2p connections for deals.

@hannahhoward hannahhoward merged commit 57ff8d6 into master Jul 2, 2020
hannahhoward added a commit that referenced this pull request Jul 2, 2020
Deal restarts make take a while particularly if there are connection timeouts. There's no reason to
run them synchronously in node initialization
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