Resume Storage Market Data Transfer #430
Conversation
* first checkpoint * draft 2 * working client restarts * more fixes * comment * document test failure
* first checkpoint * draft 2 * working client restarts * more fixes * comment * provider restarts * provider restarts * fix tests
| @@ -108,7 +109,8 @@ func NewDependenciesWithTestData(t *testing.T, ctx context.Context, td *shared_t | |||
| dtTransport1 := dtgstransport.NewTransport(td.Host1.ID(), gs1) | |||
| dt1, err := dtimpl.NewDataTransfer(td.DTStore1, td.DTNet1, dtTransport1, td.DTStoredCounter1) | |||
| require.NoError(t, err) | |||
| err = dt1.Start(ctx) | |||
| testutil.StartAndWaitForReady(ctx, t, dt1) | |||
There was a problem hiding this comment.
Hey you don't actually need to use this version -- the function is already present in the shared_testutil for go-fil-markets and will work with go-data-transfer. I should probably extract that whole functionality to common utilities repo at some point :)
storagemarket/dealstatus.go
Outdated
| @@ -65,6 +65,14 @@ const ( | |||
| // StorageDealTransferring means data is being sent from the client to the provider via the data transfer module | |||
| StorageDealTransferring | |||
|
|
|||
| // StorageDealProviderTransferRestart means a storage deal data transfer from client to provider will be restarted | |||
There was a problem hiding this comment.
at this point, we should put these at the end of the states list -- people are starting to rely on the deal states list not changing
Address review feedback
Codecov Report
@@ Coverage Diff @@
## master #430 +/- ##
==========================================
- Coverage 66.33% 66.05% -0.27%
==========================================
Files 48 48
Lines 3189 3237 +48
==========================================
+ Hits 2115 2138 +23
- Misses 830 850 +20
- Partials 244 249 +5
Continue to review full report at Codecov.
|
| }), | ||
|
|
||
| fsm.Event(storagemarket.ClientEventDataTransferRestarted). | ||
| FromMany(storagemarket.StorageDealClientTransferRestart, storagemarket.StorageDealStartDataTransfer).To(storagemarket.StorageDealTransferring). |
There was a problem hiding this comment.
This should not ideally run when in StorageDealStartDataTransfer right? Why is this in the list?
There was a problem hiding this comment.
We now move from StorageDealStartDataTransfer to StorageDealTransferring only after we see the Accept from the other side.
However, what if the other side sends an accept, we crash before receiving it and then restart. It means the channel exists on both sides and yet, we are restarting in the StorageDealClientTransferRestart state.
| }), | ||
|
|
||
| fsm.Event(storagemarket.ProviderEventDataTransferRestarted). | ||
| FromMany(storagemarket.StorageDealWaitingForData, storagemarket.StorageDealProviderTransferRestart).To(storagemarket.StorageDealTransferring). |
There was a problem hiding this comment.
What's the scenario where this is called in StorageDealWaitingForData?
There was a problem hiding this comment.
If the provider crashes after the channel is open on it's side but before seeing the corresponding event in the ProviderDataTransferSubscriber, it will never enter the StorageDealTransferring state.
| @@ -102,27 +103,30 @@ type MinerDeal struct { | |||
|
|
|||
| DealID abi.DealID | |||
| CreationTime cbg.CborTime | |||
|
|
|||
| TransferChannelId *datatransfer.ChannelID | |||
There was a problem hiding this comment.
On the provider side, we use a pointer to a channelID, while on the client side, we use a channelID value -- what's the rationale for the difference?
There was a problem hiding this comment.
I think this is an oversight. Let's use pointer on both sides.
PR to merge data transfer work to master.