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

fix restarts during data transfer for a retrieval deal #540

Merged
merged 5 commits into from
May 6, 2021

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Apr 19, 2021

Fixes #533
Supersedes #539
Depends on filecoin-project/go-data-transfer#197

TODO:

  • Clean up logging & commented out code
  • Handle case where transfer is restarted in validatePull()
  • Manual test

@dirkmc dirkmc force-pushed the fix/retrieval-restarts branch 3 times, most recently from 582d918 to f696914 Compare April 19, 2021 15:23
@dirkmc dirkmc changed the title wip: fix restarts during data transfer for a retrieval deal fix restarts during data transfer for a retrieval deal Apr 19, 2021
@dirkmc dirkmc force-pushed the fix/retrieval-restarts branch 8 times, most recently from 5812917 to 03977c6 Compare April 22, 2021 13:52
@dirkmc dirkmc marked this pull request as ready for review April 22, 2021 15:30
Copy link
Collaborator

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

@dirkmc

First round of comments/questions.

@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2021

Codecov Report

Merging #540 (25f4f77) into master (be1da4a) will increase coverage by 0.23%.
The diff coverage is 83.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
+ Coverage   65.54%   65.77%   +0.23%     
==========================================
  Files          53       56       +3     
  Lines        3549     3634      +85     
==========================================
+ Hits         2326     2390      +64     
- Misses        990     1009      +19     
- Partials      233      235       +2     
Impacted Files Coverage Δ
retrievalmarket/dealstatus.go 0.00% <0.00%> (ø)
retrievalmarket/events.go 0.00% <0.00%> (ø)
...etrievalmarket/impl/providerstates/provider_fsm.go 6.46% <0.00%> (ø)
...mpl/requestvalidation/unified_request_validator.go 81.82% <ø> (ø)
storagemarket/events.go 37.50% <37.50%> (ø)
retrievalmarket/impl/clientstates/client_fsm.go 68.60% <85.72%> (-4.05%) ⬇️
...rievalmarket/impl/requestvalidation/revalidator.go 76.97% <89.29%> (+1.80%) ⬆️
retrievalmarket/impl/clientstates/client_states.go 94.34% <100.00%> (+1.94%) ⬆️
retrievalmarket/impl/dtutils/dtutils.go 79.44% <100.00%> (ø)
retrievalmarket/impl/provider_environments.go 55.96% <100.00%> (+0.54%) ⬆️
... and 6 more

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 b080c6f...25f4f77. Read the comment docs.

Copy link
Collaborator

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

@dirkmc Have finished the review. Some questions/concerns.

Astounding work !

Copy link
Collaborator

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

Just one comment. LGTM.

retrievalmarket/impl/provider_environments.go Show resolved Hide resolved
@dirkmc dirkmc merged commit ad3d7d3 into master May 6, 2021
@dirkmc dirkmc deleted the fix/retrieval-restarts branch May 6, 2021 07:27
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.

Integration test: retrieval deal restart during transfer
3 participants