Don't Keep Streams Open #230
Conversation
hannahhoward
force-pushed
the
feat/close-streams
branch
from
8b0fbca to
704ed4e
Compare
hannahhoward
force-pushed
the
feat/close-streams
branch
from
704ed4e to
061890f
Compare
Codecov Report
@@ Coverage Diff @@
## master #230 +/- ##
==========================================
- Coverage 66.03% 65.48% -0.54%
==========================================
Files 40 39 -1
Lines 2269 2242 -27
==========================================
- Hits 1498 1468 -30
- Misses 657 660 +3
Partials 114 114
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks good; my rebase actually does look a little hairier. But that's ok.
One comment about naming; I think SignedResponse was originally intended to be used for when we had a counter signature from the Provider. But now that is handled by the PublishStorageDeals message, this is more accurately a StorageDealStatus or a ProviderDealStatus or something like that. I'm mainly thinking about this because in my branch there are multiple "responses" so the request/response naming makes less sense. It can wait though for sure.
| @@ -305,7 +304,7 @@ func TestEnsureProviderFunds(t *testing.T) { | |||
| }, | |||
| dealInspector: func(t *testing.T, deal storagemarket.MinerDeal) { | |||
| require.Equal(t, storagemarket.StorageDealProviderFunding, deal.State) | |||
| require.Equal(t, cids[0], deal.AddFundsCid) | |||
| require.Equal(t, &cids[0], deal.AddFundsCid) | |||
There was a problem hiding this comment.
I am really confused about pointers, equality and how this works with serialization into and out of the store. I guess maybe in this case require.Equal is dereferencing the pointers under the hood?
I guess a more general question, why do we have a mix of *cid.Cid and cid.Cid in the MinerDeal and ClientDeal? Should we be using one or the other?
There was a problem hiding this comment.
yes -- good point, non-obvious, but this is knowledge that should be shared by everyone. A CID that is not ALWAYS set should be a pointer. If a data structure has a regular cid with a value of cid.Undef, it will NOT serialize to CBOR. These changes are actually corrections to already merged code that had to be made once I ran our cbor generation scripts. I'm working. @shannonwells is working on a ticket to add checking that CBOR generation has been run and is up to date to our CI pipeline.
Goals
Our approach has been to just leave streams open, and read form them at will in storage market, and this has caused several issues.
This PR moves to closing streams as soon as we send information in the storage market. Retrieval will move to this also, but only after the data transfer move has happened
Implementation
Follow-up