Skip to content

feat: check paths completed for all retrieval types#386

Closed
rvagg wants to merge 21 commits intomainfrom
rvagg/path-too-far-fail
Closed

feat: check paths completed for all retrieval types#386
rvagg wants to merge 21 commits intomainfrom
rvagg/path-too-far-fail

Conversation

@rvagg
Copy link
Copy Markdown
Member

@rvagg rvagg commented Aug 18, 2023

Depends on filecoin-project/go-data-transfer#378 for the graphsync part of this. Also includes a little bit of cleanup that makes the diff noisier.

Because traversals don't strictly need to run to any pre-defined completion, we have to check it ourselves - we watch the LastPath in the traversal progress and then check whether it at least fulfils our initial path request.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 18, 2023

Codecov Report

❌ Patch coverage is 77.27273% with 105 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.39%. Comparing base (f31fcb4) to head (88ccb73).
⚠️ Report is 166 commits behind head on main.

Files with missing lines Patch % Lines
...ernal/itest/linksystemutil/linksystemblockstore.go 22.00% 35 Missing and 4 partials ⚠️
pkg/internal/itest/testpeer/peerhttpserver.go 64.94% 26 Missing and 8 partials ⚠️
cmd/lassie/fetch.go 28.57% 9 Missing and 1 partial ⚠️
pkg/verifiedcar/verifiedcar.go 87.65% 6 Missing and 4 partials ⚠️
pkg/types/types.go 79.06% 6 Missing and 3 partials ⚠️
pkg/internal/itest/testpeer/generator.go 92.50% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
+ Coverage   77.33%   77.39%   +0.05%     
==========================================
  Files          86       87       +1     
  Lines        6423     6608     +185     
==========================================
+ Hits         4967     5114     +147     
- Misses       1199     1237      +38     
  Partials      257      257              
Files with missing lines Coverage Δ
pkg/internal/itest/mocknet/mocknet.go 78.03% <100.00%> (ø)
pkg/internal/testutil/mockclient.go 55.55% <100.00%> (+1.20%) ⬆️
pkg/internal/testutil/toblocks.go 75.67% <100.00%> (-0.65%) ⬇️
pkg/lassie/lassie.go 73.94% <ø> (ø)
pkg/retriever/bitswapretriever.go 95.65% <100.00%> (+1.63%) ⬆️
pkg/retriever/graphsync/client/client.go 68.01% <100.00%> (ø)
pkg/retriever/graphsyncretriever.go 86.41% <100.00%> (+0.70%) ⬆️
pkg/retriever/httpretriever.go 85.83% <100.00%> (+0.11%) ⬆️
pkg/retriever/parallelpeerretriever.go 93.36% <100.00%> (ø)
pkg/retriever/retriever.go 89.51% <ø> (ø)
... and 10 more

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rvagg rvagg marked this pull request as draft August 22, 2023 00:35
@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Aug 22, 2023

Changed to Draft while we consider strategy - maybe via the daemon you don't need/want to signal an error as you technically fulfilled the request and it's up to a upstream party to decide that this is an error. On the CLI we probably want to let the user know.

Also, maybe the error shouldn't be per retriever, perhaps it ought to be validated after the retrieval occurs--in which case maybe the RetrievalStats should include a LastPath from the traversal so we can easily verify it outside of the process?

@rvagg rvagg force-pushed the rvagg/byte-range-proper branch from ad5766c to c40fe59 Compare August 23, 2023 00:16
@rvagg rvagg force-pushed the rvagg/path-too-far-fail branch from 79b6f80 to 88ccb73 Compare August 23, 2023 02:45
@rvagg rvagg force-pushed the rvagg/byte-range-proper branch 2 times, most recently from ba2f0b7 to 08d6372 Compare August 24, 2023 06:32
Base automatically changed from rvagg/byte-range-proper to main August 24, 2023 07:15
@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Sep 6, 2023

some of this will be included in #404, we can opt for checking lassie fetch retrievals and reporting to the user when we have that functionality available, it'll just be a matter of wiring up the pieces

@rvagg rvagg closed this Sep 6, 2023
@rvagg rvagg deleted the rvagg/path-too-far-fail branch September 6, 2023 03:15
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