Add an HTTP retrieval protocol #193
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #193 +/- ##
==========================================
+ Coverage 70.86% 71.16% +0.30%
==========================================
Files 63 68 +5
Lines 5307 6063 +756
==========================================
+ Hits 3761 4315 +554
- Misses 1348 1505 +157
- Partials 198 243 +45
|
| clock clock.Clock, | ||
| awaitReceivedCandidates chan<- struct{}, | ||
| initialPause time.Duration, | ||
| customCompare func(a, b ComparableCandidate, mda, mdb metadata.Protocol) bool, |
There was a problem hiding this comment.
are these worth being options? maybe a subsequent refactor
There was a problem hiding this comment.
Yeah, so this is NewHttpRetrieverWithDeps() which is intended for instantiating for testing. NewHttpRetriever() is the normal path with defaults. We've been using this pattern in a few places, including NewGraphsyncRetrieverWithConfig and NewBitswapRetrieverFromDeps.
The customCompare is particularly shameful because it's only a testing option so we get to control candidate order. #197 #92 should make this go away. Also if we move logic into Connect() then we have something to measure (and control) at least, like we do with Graphsync.
| return a.Duration < b.Duration | ||
| } | ||
|
|
||
| func (ph *ProtocolHttp) Connect(ctx context.Context, retrieval *retrieval, candidate types.RetrievalCandidate) error { |
There was a problem hiding this comment.
one option is to start the net.Dial or tls.Dial at this point, so that you then have that dialing time to use for comparing candidates
There was a problem hiding this comment.
I was wondering if something like this was possible (hence my questions to you about using libp2p's http client). I might need some pointers on how to achieve the desired result. Having something to put in Connect() would help both warm things up but also give us some timing data to help with priorisation.
| log.Warnf("Couldn't construct a http request %s: %v", candidate.MinerPeer.ID, err) | ||
| return nil, fmt.Errorf("%w for peer %s: %v", ErrBadPathForRequest, candidate.MinerPeer.ID, err) | ||
| } | ||
| req.Header.Add("Accept", request.Scope.AcceptHeader()) |
There was a problem hiding this comment.
are there user agent / other headers that should be here?
rvagg
force-pushed
the
feat/http
branch
2 times, most recently
from
c94e0de
to
6810ff2
Compare
based on bitswap unit test framework
* too-detailed http testing, will remove this in favour of graphsync style testing.
* feat(verifiedcar): initial verifiedcar package * feat(verifiedcar): verify http retrievals * chore(verifiedcar): tests for basic error cases * fix(verifiedcar): coverage of more cases, handle known edges properly * fix(verifiedcar): remove extraneous go-routine (#226) Co-authored-by: Rod Vagg <[email protected]> * fix(verifiedcar): address feedback * fix(verifiedcar): fix flaky tests --------- Co-authored-by: Hannah Howard <[email protected]>
* test: add itests for http * test: add peer http server, minor refactors and fixes
remaining work in my head: