Network Abstraction For Retrieval Market #17
Conversation
shannonwells
force-pushed
the
feat/retrieval-network
branch
from
939ce0a
to
4a82c87
Compare
Codecov Report
@@ Coverage Diff @@
## master #17 +/- ##
=========================================
Coverage ? 14.97%
=========================================
Files ? 26
Lines ? 1756
Branches ? 0
=========================================
Hits ? 263
Misses ? 1445
Partials ? 48
Continue to review full report at Codecov.
|
| @@ -3,6 +3,7 @@ package filestore | |||
| import ( | |||
There was a problem hiding this comment.
I think this is another rebase that's in a weird place
There was a problem hiding this comment.
This file is still in master. The changes are because the linter complained about not error checking
shannonwells
force-pushed
the
feat/retrieval-network
branch
from
ff1746a
to
4da6c81
Compare
There was a problem hiding this comment.
Wow
So this PR turned out really awesome!
I can't mark it approved cause I opened the PR I guess :)
I have a couple non-blocking comments you can address if you like. Overall though, nice work and thanks for all the detailed testing and additional fixtures and what not.
| @@ -0,0 +1,109 @@ | |||
| package shared_testutil | |||
| } | ||
|
|
||
| func TestQueryStreamSendReceiveOutOfOrderFails(t *testing.T) { | ||
| // send query, read response in handler - fails |
There was a problem hiding this comment.
I had in mind just doing:
from: write query
to: read query response <- errors
Not the second step as well. It's fine that the test is actually testing more, but if it means you can get rid of the go routine I'd probably just do that.
| assert.Equal(t, dpy, receivedPayment) | ||
| } | ||
|
|
||
| func TestQueryStreamSendReceiveMultipleOutOfOrderFails(t *testing.T) { |
There was a problem hiding this comment.
This is my own fault, but, honestly, this test doesn't make sense in retrospect.
I think the reason you're deadlocking without a go routine is the second WriteDealProposal never finishes in side the go routine. But cause there's no channel write at the end of the go routine, you just never see that.
It would be interesting to put a channel write at the end of the go routine and then try to read from it at the very end. I think you might find it never gets written to.
Assuming that's the case, I think this test gives a false impression of testing something it doesn't. Namely that all the writes after the first do not in fact pass.
I'm not sure if there's a good way to resolve this, and I wonder if it's just unrealstic to trying to have this test -- in practice,
the handlers we write are likely to return as soon as they encounter an error, and there will not be multiple error reads in a row.
So my honest, though non-blocking, recommendation is to delete this test and simplify the out-of-order query read test above to just test a single write/read, so that we don't have to deal with this. Sorry for having unneccesary work.
| "github.com/filecoin-project/go-fil-components/retrievalmarket" | ||
| ) | ||
|
|
||
| type DealStream struct { |
There was a problem hiding this comment.
NB: I wonder if this type should be private to the module. is there any downside there?
* extract mocknet setup from datatransfer testing, fold whitebox testing into graphsync_impl_test
Extract all components of storage market
Create a network to wrap cbor read/writes and introduce some typechecking, and provide a mockable abstraction
* extract mocknet setup from datatransfer testing, fold whitebox testing into graphsync_impl_test
Create a network to wrap cbor read/writes and introduce some typechecking, and provide a mockable abstraction
* DealStream implementations
shannonwells
force-pushed
the
feat/retrieval-network
branch
from
90417f8
to
aee364d
Compare
To Simplify RetrievalCode and improve testability, this creates a retrieval market network abstraction to handle the protocols
Interfaces are here, implementation to come