-
Notifications
You must be signed in to change notification settings - Fork 59
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
Network Abstraction For Retrieval Market #17
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is another rebase that's in a weird place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is still in master. The changes are because the linter complained about not error checking
ff1746a
to
4da6c81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat!
} | ||
|
||
func TestQueryStreamSendReceiveOutOfOrderFails(t *testing.T) { | ||
// send query, read response in handler - fails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: I wonder if this type should be private to the module. is there any downside there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve of this PR.
* 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
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